[engineering] run `npm i` during postinstall in parallel #252068

tmm1 · 2025-06-22T06:19:27Z

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

tmm1 · 2025-06-22T21:37:33Z

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 · 2025-06-23T10:16:04Z

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,

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']);

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 · 2025-06-23T13:17:55Z

opened #252171 for discussion on the dependency structure.

wraithgar · 2025-06-24T17:40:54Z

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 s 8000 eems 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 npm@11.3.0 so folks would need to be on that version at least to benefit from this.

deepak1556 · 2025-06-24T18:00:22Z

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

👍

dcq01

Hi! I'm a grad student working on a research project about using large language models to automate code review. Based on your commit d962a12 and the changes in build/npm/postinstall.js, my tool generated this comment:

Null Checks for opts: The opts parameter is spread into a new object without checking if it is null or undefined. Consider adding a check to ensure opts is an object before spreading it.
User Info Handling: Validate that the uid and gid properties from os.userInfo() exist before using them.
Error Handling: The patch does not introduce any error handling for the run function calls. It would be beneficial to include error handling to catch potential issues during command execution.
Concatenation of PATH: Ensure that opts.env[pathVar] is defined before concatenating. If opts.env[pathVar] is undefined, the resulting opts.env[pathVar] will only be set to the new path.
Potential for Path Delimiter Issues: If opts.env[pathVar] is undefined, the concatenation will not include a delimiter. It might be safer to conditionally add the delimiter only if opts.env[pathVar] is defined.
Environment Variable Case Sensitivity: The patch correctly handles the case sensitivity of the PATH variable on Windows by using Path instead of PATH.
Redundant Path Construction: Store the path in a variable and reuse it to avoid redundancy:
```
```javascript
```

const nodeGypPath = process.platform === 'win32'

    ? path.join(__dirname, 'gyp', 'node_modules', '.bin', 'node-gyp.cmd')

    : path.join(__dirname, 'gyp', 'node_modules', '.bin', 'node-gyp');

opts.env['npm_config_node_gyp'] = nodeGypPath;

opts.env[pathVar] = nodeGypPath + path.delimiter + opts.env[pathVar];

```
```
```
String Concatenation: Use template literals for better readability:
opts.env[pathVar] = ${path.join(__dirname, 'gyp', 'node_modules', '.bin')}${path.delimiter}${opts.env[pathVar]};
Logging Sensitive Information: Ensure that sensitive data, such as GITHUB_TOKEN, is not logged or is masked 80AB appropriately to prevent leakage.

As part of my research, I'm trying to understand how useful these comments are in real-world development. If you have a moment, I'd be super grateful if you could quickly reply to these two yes/no questions:

Does this comment provide suggestions from a dimension you hadn’t considered?
1. Do you find this comment helpful?

Thanks a lot for your time and feedback! And sorry again if this message is a bother.

tmm1 added 4 commits June 21, 2025 23:15

npmrc: fix config for node_gyp

80f9c3e

build/npm: run postinstall in parallel

664824b

set PATH to ensure bundled node-gyp usage

4167350

ensure node_gyp config is set for all npm-install invocations

3689bd3

vs-code-engineering bot assigned deepak1556 Jun 22, 2025

tmm1 added 2 commits June 22, 2025 00:21

fix

19376f8

fix win32 path

d962a12

deepak1556 added this to the June 2025 milestone Jun 23, 2025

deepak1556 mentioned this pull request Jun 23, 2025

Restructure project npm dependencies #252171

Open

wraithgar mentioned this pull request Jun 24, 2025

chore: use node-gyp from build dir for the project #250981

Merged

dcq01 reviewed Jun 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[engineering] run `npm i` during postinstall in parallel #252068

[engineering] run `npm i` during postinstall in parallel #252068

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[engineering] run npm i during postinstall in parallel #252068

Are you sure you want to change the base?

[engineering] run npm i during postinstall in parallel #252068

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[engineering] run `npm i` during postinstall in parallel #252068

[engineering] run `npm i` during postinstall in parallel #252068