-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Fix Typings #3280
Conversation
@todd thanks! Is there a linter for this so we can run it before release? |
And do you need a new release or just merging this in is enough? |
|
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. |
Thanks guys! I think a new release is required. Until one ships moving back to 2.13.0 should do. |
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. |
@johnnyreilly Can we just run all our tests under this? |
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. |
Oh and thanks for all the hard work x2 :-) |
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😞) |
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 😃 |
Just had a look at the code. The lowest friction way to to this would probably be to drop in the As @todd says, you'd need to add TypeScript as a dev dependency. |
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? |
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.
Yes
Yes |
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 |
@johnnyreilly well, it won't be just |
Yes it will probably be |
Fwiw it could just be |
The TypeScript typings in 2.14.0 are incorrect and breaking our builds. This should fix the issue.
Thanks for all the hard work ❤️