Studio CLI: Add bundled Node binary and fix Windows popup #2303
Conversation
scripts/download-node-binary.sh
Outdated
There was a problem hiding this comment.
To make this more portable, let's do it in JS instead.
bcotrim
force-pushed
the
stu-1148-investigate-site-create-performance
branch
from
75bccac to
dd6bce1
Compare
bcotrim
changed the title
nightnei
left a comment
nightnei
left a comment
There was a problem hiding this comment.
The code looks great and works as expected:
- I don't see popup on windows anymore.
- But, I encounter x2 difference in time between CLI and Studio, is it expected?
macOS:
CLI: 6 sec
Studio: 14 sec
Windows:
CLI: 1:40
Studio: 2:50
scripts/download-node-binary.js
Outdated
| const nvmrcPath = path.join( __dirname, '..', '.nvmrc' ); | ||
| if ( fs.existsSync( nvmrcPath ) ) { | ||
| const version = fs.readFileSync( nvmrcPath, 'utf-8' ).trim(); | ||
| // Ensure version starts with 'v' |
There was a problem hiding this comment.
The code looks self-explanatory
| // Ensure version starts with 'v' |
src/storage/paths.ts
Outdated
| export function getBundledNodeBinaryPath(): string { | ||
| const nodeBinaryName = process.platform === 'win32' ? 'node.exe' : 'node'; | ||
|
|
||
| if ( process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test' ) { |
There was a problem hiding this comment.
I am thinking - should we keep it only for tests? To have it used in development, to work with the same as in prod, to avoid if in prod something will be unexpectedly broken?
| if ( process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test' ) { | |
| if ( process.env.NODE_ENV === 'test' ) { |
There was a problem hiding this comment.
The only time we can guarantee that the bundled node binary is available is in the production build. I flipped the logic in this function to return the bundled path only when NODE_ENV === 'production'
- ESM instead of CommonJS - Use npm modules instead of shell commands to extract archives - Linux isn't supported - fetch API instead of https module
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Great initiative! I'm very optimistic about the performance impact of this change.
I made a couple of changes to this PR:
- Refined
scripts/download-node-binary.mjsin 17e2ead - Updated
./binscripts to use the bundled node binary in 4e77d33 (meaning Electron isn't involved when users runstudiofrom the terminal, either) - Use the system-level node binary when forking CLI processes during development in f0b9873 (this addresses the performance problems @nightnei noted in #2303 (review))
With those changes in place, this PR should be good to land 👍
fredrikekelund
deleted the
stu-1148-investigate-site-create-performance
branch
Related issues
Proposed Changes
Add bundled Node.js binary - The CLI now uses a bundled Node.js binary instead of relying on the Electron run as Node.
Fix Windows terminal popup - Patched the
ps-manpackage to addwindowsHide: trueto spawn calls. This prevents a brief console window popup (showingC:\WINDOWS\system32\tasklist) when running CLI commands on Windows.Testing Instructions
Windows
npm run cli:buildnode dist/cli/main.js site create --name=test-sitemacOS
npm run cli:buildnode dist/cli/main.js site create --name=test-sitePre-merge Checklist