Fix IMiddy and ICorsOptions type definitions #200
Conversation
- integrate typings-tester into npm:test - write typings-tester tests for middy and cors options - minor typescript version bump - modernize typescript setup - provide @types/jest to allow tests to compile
There was a problem hiding this comment.
As far as I understand this looks like a great job :) I'd be really happy to merge it but it would be great to have a second opinion by somebody with TypeScript experience:
@peterjcaulfield, @joseSantacruz, @nnyegaard, @fsciuti, @remojansen 🙏
tsconfig.json
Outdated
| "noEmitHelpers": true, | ||
| "noEmitOnError": true, | ||
| "noImplicitReturns": true, | ||
| "target": "es5", |
There was a problem hiding this comment.
Is there a reason to change the target from es2015 to es5?
There was a problem hiding this comment.
oh, good catch. This was not meant to change.
Context: This branch started off as two features, this feature but then also another one which made the tests run on node v8.x - during that development I had an issue where the tests were not running on v6, I changed the value, it didn't make a difference, and I forgot to change it back.
The issue was that I'd switched the entire project over to using ts-jest at some point and it complained about some of the javascript within the project, but only on v6 - so I figured something about targeting or babel was wrong, and I was iterating through that.
I'll go ahead and change it back to es2015.
|
Expect for the one question everything looks good. This is a good addition! |
|
Thanks for the review @nnyegaard! I'll have this version bumped and merged shortly |
As per #182 this fixes the types issues on IMiddy and ICorsOptions.
before,after, andonErroron IMiddy were incorrectly configured:While
ICorsOptionsitself was optional whencors()was being called, the individual properties of it were not.The project already had the infrastructure to run tests but was missing a few things:
@types/nodewere not present, sotypings-testerwould failtest:typingswas not part of the build workflow; I've now added it tonpm test.I have examples of the tests failing:
These have slightly differing package.json setups which allow them to target the specific tests:
I chose to use the name:
foo.types.tsto indicate that the these files are verifying types. Additionally I chose to locate them in__tests__because that seemed like the most correct place. Finally, I also chose to use the jest notation (and thus include@types/jest) as that allows folks to just copy and paste their tests intofoo.types.tsand they'll be picked up bytypings-tester.I believe we have the minimum here to constitute a basis to ask folks to contribute accurate types to the project now. However I decided not to implement tests for all of the types because that would have made the commit much larger. I'm happy to contribute a few types each weekend though :)