[engineering] run npm i during postinstall in parallel

[tmm1]

this speeds up npm i considerably on all platforms

i have been using this for a few months so all the bugs should be worked out

one major issue was running node-gyp in parallel can cause random corruption nodejs/node-gyp#3165

cc @deepak1556 #250554 #250981

[tmm1]

The parallel parts work, but the node-gyp override is still problematic.

According to the npm ci docs:

These [scripts] all run after the actual installation of modules into node_modules, in order, with no internal actions happening in between

So setting up build/npm/gyp in preinstall.js doesn't help with the main npm ci at all, because it happens after node_modules has been constructed.

[deepak1556]

So setting up build/npm/gyp in preinstall.js doesn't help with the main npm ci at all, because it happens after node_modules has been constructed.

Agreed, an option would be to check-in node-gyp into the source tree locked to a specific version similar to how it is bundled for npm. This would avoid any preinstall script dependency but comes with it own maintenance cost.

How about moving the native module dependencies of the root folder into a subfolder say electron that would only get installed as part of the postinstall step

vscode
  |
  |-> electron
        |-> .npmrc
        |-> package.json (native module dependencies of root)

The following additionally need to be covered,

1. When running out of sources we need to inject the module paths, we already have infra for this to support our remote case

   vscode/src/bootstrap-node.ts

   Lines 59 to 76 in f20278e

   	/**
   	* Add support for redirecting the loading of node modules
   	*
   	* Note: only applies when running out of sources.
   	*/
   	export function devInjectNodeModuleLookupPath(injectPath: string): void {
   	if (!process.env['VSCODE_DEV']) {
   	return; // only applies running out of sources
   	}
   	if (!injectPath) {
   	throw new Error('Missing injectPath');
   	}
   	// register a loader hook
   	const Module = require('node:module');
   	Module.register('./bootstrap-import.js', { parentURL: import.meta.url, data: injectPath });
   	}

   and

   vscode/src/server-main.ts

   Lines 241 to 245 in f20278e

   	if (process.env['VSCODE_DEV']) {
   	// When running out of sources, we need to load node modules from remote/node_modules,
   	// which are compiled against nodejs, not electron
   	process.env['VSCODE_DEV_INJECT_NODE_MODULE_LOOKUP_PATH'] = process.env['VSCODE_DEV_INJECT_NODE_MODULE_LOOKUP_PATH'] || path.join(__dirname, '..', 'remote', 'node_modules');
   	devInjectNodeModuleLookupPath(process.env['VSCODE_DEV_INJECT_NODE_MODULE_LOOKUP_PATH']);

2. At build time ensure the modules from electron are packaged as part of the application via

   vscode/build/gulpfile.vscode.js

   Lines 291 to 312 in f20278e

   	const deps = gulp.src(dependenciesSrc, { base: '.', dot: true })
   	.pipe(filter(['**', `!**/${config.version}/**`, '!**/bin/darwin-arm64-87/**', '!**/package-lock.json', '!**/yarn.lock', '!**/*.{js,css}.map']))
   	.pipe(util.cleanNodeModules(path.join(__dirname, '.moduleignore')))
   	.pipe(util.cleanNodeModules(path.join(__dirname, `.moduleignore.${process.platform}`)))
   	.pipe(jsFilter)
   	.pipe(util.rewriteSourceMappingURL(sourceMappingURLBase))
   	.pipe(jsFilter.restore)
   	.pipe(createAsar(path.join(process.cwd(), 'node_modules'), [
   	'**/*.node',
   	'**/@vscode/ripgrep/bin/*',
   	'**/node-pty/build/Release/*',
   	'**/node-pty/build/Release/conpty/*',
   	'**/node-pty/lib/worker/conoutSocketWorker.js',
   	'**/node-pty/lib/shared/conout.js',
   	'**/*.wasm',
   	'**/@vscode/vsce-sign/bin/*',
   	], [
   	'**/*.mk',
   	'!node_modules/vsda/**' // stay compatible with extensions that depend on us shipping `vsda` into ASAR
   	], [
   	'node_modules/vsda/**' // retain copy of `vsda` in node_modules for internal use
   	], 'node_modules.asar'));

But this seems like a good path forward, considering npm/cli#8153 coming our way which requires us to inject the npm config env variables rather than them being auto injected via the rc file we need a way for the root native modules to be adapted and this seems like a good solution to cover both. Thoughts ?

[deepak1556]

opened #252171 for discussion on the dependency structure.

[wraithgar]

Added a comment to the PR for the node-gyp config but perhaps it's better to continue here. The syntax in the .npmrc file is wrong as-is.

But this seems like a good path forward, considering npm/cli#8153 coming our way which requires us to inject the npm config env variables rather than them being auto injected via the rc file we need a way for the root native modules to be adapted and this seems like a good solution to cover both. Thoughts ?

node-gyp is valid npm config so this is not a concern. There are other values in this project's .npmrc that are valid npm config and will need to be addressed, but node-gyp is not one of them.

The node-gyp config was fixed in npm recently via npm/run-script#230 and was included in [email protected] so folks would need to be on that version at least to benefit from this.

[deepak1556]

@wraithgar thanks for the context, we did notice the config name was incorrect in #250554 (comment) :)

node-gyp is valid npm config so this is not a concern. There are other values in this project's .npmrc that are valid npm config and will need to be addressed, but node-gyp is not one of them.

👍