feat: Add a shrinkwrap file to the NPM release by edvincent · Pull Request #5010 · coder/code-server · GitHub

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.0 as per the yarn.lock
argon2@0.28.5 => argon2@0.28.4 as per the yarn.lock

For 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 to jq - but a bit simpler for this use-case. And also, opens the door to not have to require jq from being installed, as this is a pure NPM package that can be declared as a dependency. Might check later if I can easily migrate every jq command to it.

I tried using npm shrinkwrap directly, but the current NPM and NodeJS versions being used seem to choke when resolutions is 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...?

Codecov Report

Merging #5010 (39350e9) into main (e1c1ba8) will not change coverage.
The diff coverage is n/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)

Nice work and thank you for doing this! 👏🏼

I'll defer the approval to @code-asher

Suggested change

      
              # HACK: The shrinkwrap file will contain the devDependencies, which by default
          
              # HACK@edvincent: The shrinkwrap file will contain the devDependencies, which by default

Easier to know who this came while scanning this code.

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?

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 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)

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.

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.

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/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?

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/logging package 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 😛

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

    
           vscode_yarn() { 
        
             echo 'Installing Code dependencies...' 
        
             cd lib/vscode 
        
             yarn --production --frozen-lockfile 
        
             symlink_asar 
        
             cd extensions 
        
             yarn --production --frozen-lockfile 
        
             for ext in */; do 
        
               ext="${ext%/}" 
        
               echo "extensions/$ext: installing dependencies" 
        
               cd "$ext" 
        
               yarn --production --frozen-lockfile 
        
               cd "$OLDPWD" 
        
             done 
        
           }

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. 😆

#5056

edvincent requested a review from a team March 22, 2022 06:40

edvincent force-pushed the shrinkwrap branch from 5db6ab9 to 3666d74 Compare March 22, 2022 06:46

jsjoeio assigned jsjoeio and code-asher Mar 22, 2022

jsjoeio reviewed Mar 23, 2022

View reviewed changes

edvincent force-pushed the shrinkwrap branch 2 times, most recently from 9059d30 to 58e518f Compare March 23, 2022 18:46

feat: Add a shrinkwrap file to the NPM release

39350e9

edvincent force-pushed the shrinkwrap branch from 58e518f to 39350e9 Compare March 26, 2022 20:46

code-asher approved these changes Mar 29, 2022

View reviewed changes

Comment thread docs/install.md Outdated

Remove extra space in npm link

43fd193

code-asher merged commit 326a1d1 into coder:main Mar 29, 2022

TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022

feat: Add a shrinkwrap file to the NPM release (coder#5010 )

ec56745

jsjoeio mentioned this pull request May 31, 2022

Update termux.md #5233

Closed

+                # an npm-shrinkwrap file from our yarn lockfile and the current node-modules installed.
+                synp --source-file yarn.lock
+                npm shrinkwrap
+                # HACK: The shrinkwrap file will contain the devDependencies, which by default

	# HACK: The shrinkwrap file will contain the devDependencies, which by default
	# HACK@edvincent: The shrinkwrap file will contain the devDependencies, which by default

feat: Add a shrinkwrap file to the NPM release by edvincent · Pull Request #5010 · coder/code-server · GitHub

Repository navigation

feat: Add a shrinkwrap file to the NPM release - #5010

feat: Add a shrinkwrap file to the NPM release#5010code-asher merged 2 commits intocoder:maincoder/code-server:mainfrom edvincent:shrinkwrapedvincent/code-server:shrinkwrapCopy head branch name to clipboard

Conversation

edvincent commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jsjoeio commented Mar 23, 2022

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

jsjoeio Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

edvincent Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

jsjoeio Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edvincent commented Mar 23, 2022

Uh oh!

jsjoeio commented Mar 23, 2022

Uh oh!

jsjoeio commented Mar 28, 2022

Uh oh!

code-asher commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

code-asher commented Mar 29, 2022

Uh oh!

jsjoeio commented Mar 29, 2022

Uh oh!

code-asher commented Mar 31, 2022

Uh oh!

code-asher commented Mar 31, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: Add a shrinkwrap file to the NPM release#5010
code-asher merged 2 commits into
coder:mainfrom
edvincent:shrinkwrap

edvincent commented Mar 22, 2022 •
edited
Loading

codecov Bot commented Mar 22, 2022 •
edited
Loading

code-asher commented Mar 29, 2022 •
edited
Loading