-
Notifications
You must be signed in to change notification settings - Fork 42
Migrate from TSLint to ESLint; update few other deps to latest #335
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
Conversation
d1a41d1 to
0442516
Compare
7b18b13 to
fe1ff1c
Compare
| "js-yaml": "^3.13.1", | ||
| "json-pointer": "^0.6.2", | ||
| "json-refs": "^3.0.13", | ||
| "json-refs": "^3.0.15", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you bumping all these patch versions? Do we need fixes only in the newer versions? If not, you should leave these alone and only update package-lock.json. It doesn't hurt to update package.json but it should be unnecessary, and is typically only updated when adding a new package, or an update is necessary for security/correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeharder what is the downside of bumping versions in package.json (as opposed to doing it only in package-lock.json) ?
I bumped them because I thought it is a good practice to try to stay at the latest as much as possible. For example, we discussed this when writing this guidance:
I understand that if this was widely used project it could possibly lead to some confusion, but given it is mostly just the 2 of us, nobody will get confused.
Also, re:
and is typically only updated when adding a new package
I added eslint packages :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a reusable package (like @azure/core-rest-pipeline), it's bad since it artificially constrains the dependency versions you are compatible with. The ecosystem works best when most packages float from the lowest version they are actually compatible with (e.g. ^2.0.0). To minimize cases where packages have incompatible dep ranges, and you need to install multiple versions side-by-side.
However, to guarantee correctness, this requires "min-max testing", where you run tests twice, once with the lowest and once with the highest compatible versions. Otherwise you can accidentally take a dependency on an API added in a new minor version, and break consumers who pinned the older minor version.
So for a package that's more of a "standalone application" like openapi-diff, where it's not worth doing min-max testing, I think you're correct that either of these strategies would be better:
-
When publishing a new version of your package, update all deps in
package.jsonto float starting from their current version (e.g.^1.2.3). This guarantees you aren't using an API from a newer version, since there is no newer version. -
Float with
~instead of^, and only update when you need to use an API in a newer version (or if a dep has a security fix, etc). If your dependencies follow semver correctly, new APIs should bump the minor version, which will require you to update your dep version before you can call the new API.
Between these two options, I would say 1 is more work to keep updated, but it should minimize the issue of side-by-side deps if someone does use you as a reusable package instead of an app. The main advantage of option 2 is less churn in package.json.
@typespec/compiler appears to use option 2, though I'm not sure how intentionally.
I'm fine using option 1 for openapi-diff (as you did here). But we'd also probably be fine to just float the lowest possible (e.g. ^1.0.0 instead of ^1.2.3). In practice, the majority of consumers will just install the latest floated versions of everything anyway, so it won't even matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeharder awesome, thank you for the detailed explanation! I added your comment to my personal reference of valuable GH comments :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, none of this probably matters for oav. If we want to update package.json periodically or every time we ship, fine. If we want less churn, only update dep versions when needed.
| }, | ||
| "description": "OpenApi Specification Diff tool", | ||
| "license": "MIT", | ||
| "type": "module", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused this bug:
https://devdiv.visualstudio.com/DevDiv/_git/openapi-alps/pullrequest/559801
##[error]Error [ERR_REQUIRE_ESM]: require() of ES Module /mnt/vss/_work/_tasks/AzureApiValidationTest_1a18ed4f-f3bf-4f34-9fed-13cb57bd2410/0.0.440/common/temp/node_modules/.pnpm/@azure+oad@0.10.11/node_modules/@azure/oad/dist/index.js from /mnt/vss/_work/_tasks/AzureApiValidationTest_1a18ed4f-f3bf-4f34-9fed-13cb57bd2410/0.0.440/private/azure-swagger-validation/azureSwaggerValidation/dist/main.js not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead either rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /mnt/vss/_work/_tasks/AzureApiValidationTest_1a18ed4f-f3bf-4f34-9fed-13cb57bd2410/0.0.440/common/temp/node_modules/.pnpm/@azure+oad@0.10.11/node_modules/@azure/oad/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).
Likely because azure-swagger-validation/azureSwaggerValidation/package.json is in CommonJS as deduced from "main": "dist/index.js",.
Related doc:
and a bonus blog post:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this in the review. I should have at least asked why you added this.
Addresses:
Note: I split-off prettier away from eslint per this guidance:
Proof the new eslint works in CI:
Proof that prettier works in CI: