Skip to content

feat: Handle disabled install scripts - #41

Closed
timfish wants to merge 4 commits into
mainfrom
timfish/feat/no-install-script
Closed

feat: Handle disabled install scripts#41
timfish wants to merge 4 commits into
mainfrom
timfish/feat/no-install-script

Conversation

@timfish

@timfishtimfish commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

npm is changing the default to no longer run install script.

This PR moves the install script logic into the side-effects of loading the module. Things become a little more complex because this library is loaded from multiple threads so we have to lock the rebuilding so it only runs once. It's tricky to do if you want to be able to recover from all the possbible failures states!

I had to inlcude a dummy install script otherwise npm and yarn v1 automatically run node-gyp rebuild at install time!

@timfish
timfishforce-pushed the timfish/feat/no-install-script branch from d7aa878 to 3ba75a5CompareJune 15, 2026 10:04
@timfishtimfish changed the title feat: No install scriptfeat: Handle install scripts disabledJun 15, 2026
@timfish
timfish marked this pull request as ready for review June 15, 2026 10:30
Comment threadsrc/copy-binary.ts
Comment threadsrc/index.ts Outdated
Comment threadsrc/index.ts Outdated
Comment threadsrc/index.ts
Comment threadpackage.json
Comment threadsrc/index.ts
Comment threadsrc/index.ts Outdated
Comment thread.github/workflows/ci.yml Outdated
Comment threadpackage.json Outdated
Comment threadsrc/index.ts
Comment threadsrc/index.ts Outdated
Comment thread.github/workflows/ci.yml
@timfish
timfishforce-pushed the timfish/feat/no-install-script branch from 536820a to 7ad75d0CompareJune 15, 2026 13:38
@timfishtimfish changed the title feat: Handle install scripts disabledfeat: Handle disabled install scriptsJun 15, 2026
Comment threadsrc/index.ts Outdated
Comment threadsrc/rebuild-lock.ts
Comment threadsrc/rebuild-lock.ts
@timfish
timfishforce-pushed the timfish/feat/no-install-script branch from 60e60b1 to 3f5c888CompareJune 15, 2026 14:47

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3f5c888. Configure here.

Comment threadsrc/rebuild-lock.ts
Comment threadsrc/index.ts
@timfish
timfish requested review from JPeer264, Lms24 and isaacsJune 16, 2026 15:21

@JPeer264JPeer264 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is now quite some acrobatics needed 😅

fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
node: [18, 26]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q/l: Is it on purpose only testing for node 18 and 26?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, didn't want to add all of them and testing oldest and newest should be enough...

Comment threadsrc/constants.ts
export const platform = process.env['BUILD_PLATFORM'] || os.platform();
export const arch = process.env['BUILD_ARCH'] || os.arch();
export const abi = getAbi(versions.node, 'node');
export const identifier = [platform, arch, stdlib, abi].filter(c => c !== undefined && c !== null).join('-');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary byte saving suggestion

Suggested change
exportconstidentifier=[platform,arch,stdlib,abi].filter(c=>c!==undefined&&c!==null).join('-');
exportconstidentifier=[platform,arch,stdlib,abi].filter(c=>c!=null).join('-');

Comment threadpackage.json
"detect-libc": "^2.0.4",
"node-abi": "^3.89.0"
"node-abi": "^3.92.0",
"node-gyp": "^11.5.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m/h: The node engines are ^18.17.0 || >=20.5.0 based on this. Not sure if this is satisfying our supported engines I'm afraid. v10 would work though, not sure if this can be considered 🤔

@timfish

Copy link
Copy Markdown
CollaboratorAuthor

Closing in favour of doing something less complex!

@timfishtimfish closed this Jun 18, 2026
@JPeer264
JPeer264 deleted the timfish/feat/no-install-script branch June 19, 2026 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timfish@JPeer264