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

Fix Typings #3280

Merged
merged 1 commit into from Jul 4, 2016

Conversation

Projects
None yet
5 participants
@todd
Contributor

todd commented Jul 4, 2016

The TypeScript typings in 2.14.0 are incorrect and breaking our builds. This should fix the issue.

Thanks for all the hard work ❤️

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2016

@todd thanks! Is there a linter for this so we can run it before release?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2016

And do you need a new release or just merging this in is enough?

@lacolaco

This comment has been minimized.

lacolaco commented Jul 4, 2016

moment.d.ts is published in the npm package and we download it from npm. It needs new version.

@todd

This comment has been minimized.

Contributor

todd commented Jul 4, 2016

It's going to have to be a new release, unfortunately 😭

You can use tslint for style-related things. Otherwise, you'll have to rely on the actual TypeScript compiler (tsc) to catch these sort of issues.

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Thanks guys! I think a new release is required. Until one ships moving back to 2.13.0 should do.

@ichernev ichernev merged commit d2c82f5 into moment:develop Jul 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@todd todd deleted the todd:fix_typings branch Jul 4, 2016

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Have you considered making use of a test usage file such as is used on Definitely Typed? Essentially you have a file which demonstrates usage of moment. If the Typings change such that usage is broken then running TSC against the Typing and the test file will result in a failing build.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2016

@johnnyreilly Can we just run all our tests under this?

@todd

This comment has been minimized.

Contributor

todd commented Jul 4, 2016

I left a comment for @ichernev on Gitter to more or less the same effect (minus the part about the test case). It would require adding TypeScript as a development dependency, though.

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Oh and thanks for all the hard work x2 :-)

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

It's a different sort of test; compilation as opposed to execution. That said, it's probably possible to combine them; though would have to look at moments setup. (On a phone right now so can't😞)

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

The more extreme step (though worth it I would suggest) is switching to writing moment in TypeScript (I don't think you do that right now?) Doing that would make this sort of error much harder to ship 😃

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Just had a look at the code. The lowest friction way to to this would probably be to drop in the moment-tests.ts file from DT. Then make the tests part of your package.json this: "test": "grunt test && tsc". This would compile the definition tests file as part of the tests. Problems would fail the build.

As @todd says, you'd need to add TypeScript as a dev dependency.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2016

tsc automatically tests a file that ends with *.ts? Can we put the tests file in another folder? Also we'd need to update the ts tests file when we make changes?

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Tsc triggers compilation. If the Typing file (moment.d.ts) no longer satisfied the usage file (moment-tests.ts) then compilation would fail which should fail the build.

Can we put the tests file in another folder?

Yes

Also we'd need to update the ts tests file when we make changes?

Yes

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

If I get a moment (ha!) I'll see if I can do a quick fork and see if I can implement this for you

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2016

@johnnyreilly well, it won't be just tsc it will be tsc path/to/tests.ts and possibly also add the moment.d.ts file in there too. That is why I was a bit surprised.

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Yes it will probably be tsc -proj typing-tests or something. That folder could be made to reference moment.d.ts where it is now or you could move moment.d.ts into the typing-tests folder and change the Typings reference in package.json

@johnnyreilly

This comment has been minimized.

Contributor

johnnyreilly commented Jul 4, 2016

Fwiw it could just be tsc if we had an accompanying tsconfig.json (which might be a good idea anyway)

@mj1856 mj1856 added this to the 2.14.1 milestone Jul 5, 2016

@mj1856 mj1856 added the TypeScript label Aug 12, 2016

ichernev added a commit that referenced this pull request Sep 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment