-
Notifications
You must be signed in to change notification settings - Fork 389
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
build(babel): transpiling with babel so we can use modern javascript #433
Conversation
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.
Are there any breaking changes from a consumer point of view? Otherwise this PR could be tagged as a refactor as well.
package.json
Outdated
"@babel/node": "^7.12.6", | ||
"@babel/plugin-proposal-optional-chaining": "^7.12.7", | ||
"@babel/preset-env": "^7.12.7", | ||
"babel-eslint": "^10.1.0", |
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.
I prefer using exact versions for dev dependencies, so they are always in sync with package lock.
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.
Doesn't that kinda defeat the purpose of semantic versioning? Seems like we would want to bring in patches automatically.
In practice, package-lock only adds value by ensuring consistent deploys between environments, so I'm not sure what benefit we get here by tracking it in master.
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.
Since we commit package lock, the same dependency versions will always be used, unless the lockfile is updated. Having exact versions in package.json helps quickly determine (if needed) what versions we're using without looking into the lockfile. For production (or non-dev) dependencies it's a bit different, as the lockfile is not part of published packages, and using version ranges lets npm install the latest compatible version and/or dedupe dependencies.
This is a matter of preference I guess, but that's why existing dev dependencies have exact versions.
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.
what versions we're using without looking into the lockfile
the lockfile does not indicate the versions of the installed packages. You have to look at the version property in package.json for the dependency inside of node_modules
. e.g. consider the case where you jump to a different commit (via pull, checkout, bisect, etc...) that has different dependencies (or versions of dependencies) set in package.json, and maybe you forgot to run npm install
after the checkout.
using version ranges lets npm install the latest compatible version and/or dedupe dependencies
Indeed, and it's really nice when this can also happen for devDependencies. I've learned this the hard way a few times over.
In order to keep the discussion on topic, I'll go ahead and remove the carets from the code in the branch for this PR, and we can figure out a good setting for save-*
in the npm config in a separate PR / issue thread
Not from the users perspective, but this does add a build step (that should in most cases happen automatically). The "!" was out of caution. Happy to remove or, or if someone else removes it, that's fine too. |
https://www.npmjs.com/package/npm-check-updates
+1 for static versions and using the lib above instead
…On Mon, 30 Nov 2020, 05:20 Rafael Bardini, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In package.json
<#433 (comment)>:
> @@ -42,6 +44,13 @@
"z-schema-errors": "^0.2.1"
},
"devDependencies": {
+ ***@***.***/cli": "^7.12.8",
+ ***@***.***/core": "^7.12.9",
+ ***@***.***/eslint-parser": "^7.12.1",
+ ***@***.***/node": "^7.12.6",
+ ***@***.***/plugin-proposal-optional-chaining": "^7.12.7",
+ ***@***.***/preset-env": "^7.12.7",
+ "babel-eslint": "^10.1.0",
Since we commit package lock, the same dependency versions will always be
used, unless the lockfile is updated. Having exact versions in package.json
helps quickly determine (if needed) what versions we're using without
looking into the lockfile. For production (or non-dev) dependencies it's a
bit different, as the lockfile is not part of published packages, and using
version ranges lets npm install the latest compatible version and/or dedupe
dependencies.
This is a matter of preference I guess, but that's why existing dev
dependencies have exact versions.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#433 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADFTUK43RSP2AM33XRHPULSSKGFVANCNFSM4UFZ3WFQ>
.
|
Best to have a good CI tool like snyk take on that task. |
Speaking out of true laziness here: I don't want to have to go into package.json and remove the caret from the beginning of any devDependency that I may end up installing. If we can agree that we want static deps for everything, let's just set I suspect the reason that there isn't a way to override npm's save-* config options for devDependencies is because there isn't any value in using a different policy for devDeps vs the other types of deps in your projects. |
It's a great PR, let's you bring it in and talk about dependencies another time aha |
🎉 This PR is included in version 3.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
in reference to issue #432 "support modern javascript", this PR aims to add support for modern javascript to this project by transpiling code with babel before it is run.