Skip to content

feat: remove the build step and load the loader from source - #200

Merged
styfle merged 5 commits into
vercel:mainfrom
yjoer:patch-4
Jul 10, 2025
Merged

feat: remove the build step and load the loader from source#200
styfle merged 5 commits into
vercel:mainfrom
yjoer:patch-4

Conversation

@yjoer

@yjoer yjoer commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

I replaced node-pre-gyp with the latest version, shrinking the dependency graph from 95 to 56 packages. Since both the regular and coverage test modes now load the loader from source, output discrepancies between them have been eliminated. Additionally, I've added integration tests for both node-pre-gyp and node-gyp-build, using bcrypt versions 5 and 6. The split chunk logic in the test project has also been updated so that each entry point now generates its own vendor chunk.

Closes #194

@yjoer
yjoer requested review from Timer and styfle as code owners July 10, 2025 19:11
@socket-security

socket-security Bot commented Jul 10, 2025

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedresolve-from@​3.0.01001008676100
Addedini@​1.3.810010010083100
Addedbcrypt@​5.1.19210010085100
Addedbcrypt@​6.0.010010010085100
Added@​mapbox/​node-pre-gyp@​2.0.09810010085100

View full report

@styfle styfle changed the title build: remove the build step and load the loader from source feat: remove the build step and load the loader from source Jul 10, 2025
main: './main.js',
chunk: './chunk.js',
bcrypt: './packages/bcrypt.js',
bcrypt5: './packages/bcrypt5.js',

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.

Should this change be a separate PR or is there a reason we need it for this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

node-pre-gyp was updated. While unit tests were already in place, I added an integration test to provide additional confidence as the foundation was laid in the previous PR. bcrypt v5 is the version that uses node-pre-gyp. And since bcrypt v5 was added, I have also included bcrypt v6.

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.

Usually I would say this should be a separate PR to prove it was passing before and then the PR to change to mapbox would prove its still passing but I think its fine to leave as is

@styfle
styfle merged commit 080ddf8 into vercel:main Jul 10, 2025
9 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants