Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade to ts 1.7.3 #3010

Merged
merged 13 commits into from
Dec 2, 2015
Merged

upgrade to ts 1.7.3 #3010

merged 13 commits into from
Dec 2, 2015

Conversation

ztsai
Copy link
Contributor

@ztsai ztsai commented Dec 2, 2015

No description provided.

@ztsai ztsai added this to the v2.0.0 milestone Dec 2, 2015
@@ -72,7 +72,7 @@ module Plottable.Animators {
* @param {number} startDelay The start delay in milliseconds.
* @returns {Easing} The calling Easing Animator.
*/
public startDelay(startDelay: number): Easing;
public startDelay(startDelay: number): this;
public startDelay(startDelay?: number): any {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are so many method return types annotated as any? shouldn't it be Easing | number here (or this | number with TS 1.7.3)? was this code never updated to use union types?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last signature is the implementation, and has to be assignable to any of the previous signatures (called "overloads"), according to the spec:

When a function has overload declarations, the overloads determine the call signatures of the type given to the function object and the function implementation signature (if any) must be assignable to that type.

https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#61-function-declarations

The problem is that number | this isn't assignable to either number or this according to the casting rules, so you'll get a compiler error ("Overload signature is not compatible with function implementation."). We can't simply leave out the any either, because that causes the error "No best common type exists among return expressions." Typescript's handbook includes an example where the implementation signature returns any.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, makes sense. looks like a shortcoming of TypeScript if we have to use any so much to achieve the nice typings for getter/setter methods like this.

@jtlan
Copy link
Contributor

jtlan commented Dec 2, 2015

@ztsai There's some that were left out:

  • Axes.Numeric: usesTextWidthApproximation()
  • Interactions.PanZoom: xScales(), yScales(), minDomainExtent(), and maxDomainExtent()
  • Plots.Line: autorangeSmooth(), downsamplingEnabled(), croppedRenderingEnabled()
  • Plots.Pie: sectorValue(), innerRadius(), outerRadius()
  • Plot: attr(), animated()
  • Plots.Scatter: size(), symbol()
  • Plots.StackedArea: downsamplingEnabled() (which currently returns Plots.Line!)

I'd do a global search for "): any" afterwards just to make sure.

@jtlan jtlan added the -1 label Dec 2, 2015
@jtlan jtlan assigned ztsai and unassigned jtlan Dec 2, 2015
@ztsai ztsai removed the -1 label Dec 2, 2015
@ztsai ztsai assigned jtlan and unassigned ztsai Dec 2, 2015
@jtlan jtlan added +1 and removed Status: In CR labels Dec 2, 2015
@jtlan jtlan removed their assignment Dec 2, 2015
ztsai added a commit that referenced this pull request Dec 2, 2015
@ztsai ztsai merged commit 29d6da7 into develop-2.0 Dec 2, 2015
@ztsai ztsai deleted the ts1.7upgrade branch December 2, 2015 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants