-
Notifications
You must be signed in to change notification settings - Fork 6.3k
feat: Add a shrinkwrap file to the NPM release #5010
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
5db6ab9 to
3666d74
Compare
Codecov Report
Merging #5010 (39350e9) into main (e1c1ba8) will not change coverage.
The diff coverage isn/a.
@@ Coverage Diff @@ ## main #5010 +/- ## ======================================= Coverage 71.30% 71.30% ======================================= Files 30 30 Lines 1683 1683 Branches 373 373 ======================================= Hits 1200 1200 Misses 413 413 Partials 70 70
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update e1c1ba8...39350e9. Read the comment docs.
npm ERR! peer dep missing: @google-cloud/logging@^4.5.2, required by @coder/logger@1.1.16
I do see this in the diff 🤔 But it seems like the build and the tests are passing so I'm not 100% if we need to worry about it or not. Any ideas on why that's happening?
Not even sure why those resolutions are needed...?
If this is referring to the resolutions key in the package.json, it's for security reasons that we use it. Even though we don't depend on the dependencies directly, this ensures that when folks install code-server, they're not installing packages with security vulnerabilities. (at least that's my understanding)
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.
Nice work and thank you for doing this! 👏🏼
I'll defer the approval to @code-asher
ci/build/build-release.sh
Outdated
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.
Easier to know who this came while scanning this code.
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.
Hahahaha you mean - easier to know who to blame? ;D But yeah makes sense.
Thoughts on the overall idea/hack of removing devDependencies? I think NPM 7/8 will fix this, but with NPM 6 it would force people to use the --production... So IMO it's a worthwile hack?
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.
LOLOL hey you said it not me 😂
I mean...it is hacky but it sounds like it's the best we can do without forcing people to use NPM 7/8. And since we're still forcing people to use Node v14, I don't think we should require NPM 7/8 otherwise you'd have to install Node v14 then upgrade NPM which sounds painful from a UX perspective.
Would love to get @code-asher's thoughts though.
I do see this in the diff 🤔 But it seems like the build and the tests are passing so I'm not 100% if we need to worry about it or not. Any ideas on why that's happening?
Since I started playing around with code-server (around 3.9.1 IIRC), I've always had this error showing - yet everything always works well.
My understanding from the code is that it works because it's technically an optional dependency of @coder/logger if you wanna log to GCP - so if it's not there, nothing bad happens. And that the behavior of peerDependencies is being leveraged to make it optional?
@code-asher can likely confirm?
I guess the nicer way to fix this is to make that extender (to log to GCP) a separate package, to avoid pulling the @google-cloud/logging package by default.
If this is referring to the
resolutionskey in thepackage.json, it's for security reasons that we use it. Even though we don't depend on the dependencies directly, this ensures that when folks install code-server, they're not installing packages with security vulnerabilities. (at least that's my understanding)
Yep that makes sense actually now that you say it. I think NPM 6 is choking on lockfiles/shrinkwraps because they are in resolutions without being in dependencies.
9059d30 to
58e518f
Compare
My understanding from the code is that it works because it's technically an optional dependency of @coder/logger if you wanna log to GCP - so if it's not there, nothing bad happens. And that the behavior of peerDependencies is being leveraged to make it optional?
Ah, that makes sense now!
I guess the nicer way to fix this is to make that extender (to log to GCP) a separate package, to avoid pulling the @google-cloud/logging package by default.
That definitely would be a nicer option. The logger work is before my time here but maybe it's something we'll fix in the future.
Yep that makes sense actually now that you say it. I think NPM 6 is choking on lockfiles/shrinkwraps because they are in resolutions without being in dependencies.
Ah! Sounds likely then.
58e518f to
39350e9
Compare
bump @code-asher
Might check later if I can easily migrate every jq command to it.
That would be brilliant.
My understanding from the code is that it works because it's technically an optional dependency of
@coder/loggerif you wanna log to GCP - so if it's not there, nothing bad happens. And that the behavior ofpeerDependenciesis being leveraged to make it optional?@code-asher can likely confirm?
Yup.
I guess the nicer way to fix this is to make that extender (to log to GCP) a separate package, to avoid pulling the
@google-cloud/loggingpackage by default.
Definitely down to do this. tbh I am tempted to just remove the GCP code from the logger since we do not even use it anywhere 😛
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.
Nice work!
Something we might want to consider in the future is to shrinkwrap Code's dependencies as well.
One thought just occurred to me, I am thinking we probably need to change the yarn commands to npm in the post-install script since we removed yarn as a dependency from the docs.
code-server/ci/build/npm-postinstall.sh
Lines 92 to 109 in 18e19d2
Which I suppose means we need to generate shrinkwraps for the Code package.json files sooner rather than later.
Which I suppose means we need to generate shrinkwraps for the Code package.json files sooner rather than later.
Nice catch! Should we open an issue for this? Happy to do so, just lmk.
I will stop being lazy and do it. 😆
Uh oh!
There was an error while loading. Please reload this page.
Fixes #4927
Tried multiple ideas/approaches, this is the least worst I could find. Happy to amend/change things, and at least this will get the discussion going.
Here's a diff from an npm install before the change (left - installing from the public repos) and after (right - installing from the tar.gz generated by the build): https://editor.mergely.com/8tpzCXG6/
As expected, some versions have been bumped down, to actually follow what the yarn.lock had:
rotating-file-stream@3.0.3=>rotating-file-stream@3.0.0as per the yarn.lockargon2@0.28.5=>argon2@0.28.4as per the yarn.lockFor this approach, I'm introducing two new devDependencies:
synp, which means we can seed the shrinkwrap from the yarn.lock - rather than maintaining two different ones.json, to do some simplifications on the shrinkwrap file. Effectively, this is similar tojq- but a bit simpler for this use-case. And also, opens the door to not have to requirejqfrom being installed, as this is a pure NPM package that can be declared as a dependency. Might check later if I can easily migrate everyjqcommand to it.I tried using
npm shrinkwrapdirectly, but the current NPM and NodeJS versions being used seem to choke whenresolutionsis used to force newer versions of transitive dependencies we don't depend on (they generate a package-lock.json without a version...). Not even sure why those resolutions are needed...?