feat: Add a shrinkwrap file to the NPM release

[edvincent]

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:

• [email protected] => [email protected] as per the yarn.lock
• [email protected] => [email protected] 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]

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.

[jsjoeio]

npm ERR! peer dep missing: @google-cloud/logging@^4.5.2, required by @coder/[email protected]

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)

[jsjoeio]

Nice work and thank you for doing this! 👏🏼

I'll defer the approval to @code-asher

[jsjoeio]

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.

[edvincent]

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?

[jsjoeio]

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.

[edvincent]

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.

[jsjoeio]

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.

[jsjoeio]

bump @code-asher

[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 😛

[code-asher]

Nice work!

Something we might want to consider in the future is to shrinkwrap Code's dependencies as well.

[code-asher]

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.

[jsjoeio]

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.

[code-asher]

I will stop being lazy and do it. 😆

[code-asher]

#5056