Conversation
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [12.x, 14.x, 16.x] |
There was a problem hiding this comment.
should we also be testing node 18.x? since its the current LTS
There was a problem hiding this comment.
also i believe restify 10 doesn't officially support 12.x
There was a problem hiding this comment.
I'm in favor of adding node 18 and dropping node 12
|
Will we publish a new version to npm once this PR is merged? trying to figure out an eta for the package upgrade 😄 |
|
Another thing to note about this PR @dcavanagh , if you are adding github ci should we not remove travis? or is the goal to have both? |
|
@PodaruDragos @acommodari I could use some help getting the tests to pass. Switched to jest and eslint as well. I published master as it is |
| "allowSyntheticDefaultImports": true, | ||
| "allowUnreachableCode": false, | ||
| "allowUnusedLabels": false, | ||
| "alwaysStrict": true, |
There was a problem hiding this comment.
A lot of these added settings don't need to be explicitly added since they fall under other settings
a good example is strict which defaults a dozen or so rules to true by default.
i also think a target of es2017 should be fine since even restify 8.x does not support versions of node below node 8 at least officially
There was a problem hiding this comment.
@acommodari I'm not really sure which one is not needed, ( I added them a long time ago back in express-utils ) but if you have the time to sort this out, can you also please add them in express-utils as well ?
3 participants
No description provided.