Repository navigation
feat: remove the build step and load the loader from source - #200
Merged
Merged
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
styfle
changed the title
styfle
reviewed
Jul 10, 2025
| main: './main.js', | ||
| chunk: './chunk.js', | ||
| bcrypt: './packages/bcrypt.js', | ||
| bcrypt5: './packages/bcrypt5.js', |
Member
There was a problem hiding this comment.
Should this change be a separate PR or is there a reason we need it for this PR?
Contributor
Author
There was a problem hiding this comment.
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.
Member
There was a problem hiding this comment.
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
approved these changes
Jul 10, 2025
|
🎉 This PR is included in version 1.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
styfle
added a commit
to vercel/ncc
that referenced
this pull request
Aug 13, 2026
Bumps `@vercel/webpack-asset-relocator-loader` from **1.7.3** to **1.10.3** (latest). ## What's in the bump | Version | Change | | --- | --- | | 1.7.4 | Use a computed `node-gyp-build` path ([#185](vercel/webpack-asset-relocator-loader#185)) | | 1.7.5 | Fix `__nccwpck_require__ is not defined` regression ([#195](vercel/webpack-asset-relocator-loader#195)) | | 1.8.0 | Support `node:path` as an alias of `path` ([#197](vercel/webpack-asset-relocator-loader#197)) | | 1.9.0 | Load native addons from the `prebuilds` directory ([#198](vercel/webpack-asset-relocator-loader#198)) | | 1.9.1 | Support the packaging format of sharp v0.34.x ([#199](vercel/webpack-asset-relocator-loader#199)) | | 1.10.0 | Ship the loader as source instead of a prebuilt bundle ([#200](vercel/webpack-asset-relocator-loader#200)) | | 1.10.2 | Packaging/CI fixes | | 1.10.3 | Percent-decode the ESM asset base ([#220](vercel/webpack-asset-relocator-loader#220)) | Because 1.10.0 publishes `src/` rather than an ncc-built `dist/`, the loader's previously bundled dependencies are now real transitive dependencies — that accounts for the lockfile growth. The `relocate-loader` build emits no new assets, and `dist/ncc/loaders/relocate-loader.js.cache.js` actually shrinks slightly (499.37KB → 492.13KB). ## Why the unit fixtures change The loader now registers the asset base through a webpack `RuntimeModule` instead of the deprecated `mainTemplate.hooks.requireExtensions`. Two consequences show up in `test/unit/*/output*.js`: 1. The block is labelled `/* webpack/runtime/asset-relocator-loader */` instead of `/* webpack/runtime/compat */`, and is emitted earlier in the runtime. 2. Bundles that never reference `__nccwpck_require__` no longer carry the dead `if (typeof __nccwpck_require__ !== 'undefined') __nccwpck_require__.ab = ...` assignment — the runtime module is only added when `RuntimeGlobals.require` is actually in the tree. The guard was always false in those bundles, so this is dead-code removal, not a behaviour change. I specifically checked that the second point is not a regression. `__nccwpck_require__.ab` is still emitted wherever it is referenced, verified by hand for CJS, ESM, and concatenated-TypeScript builds that relocate an asset, and by the asset-heavy integration tests (`sharp`, `canvas`, `ffmpeg`, `oracledb`, `leveldown`, `binary-require`), which all pass. ## The Windows fix now comes from upstream The first push was green on Linux and macOS but failed on all three Windows jobs: ``` Cannot find module 'C:/Users/RUNNER%7E1/AppData/Local/Temp/.../runtime.json' ``` The relocator derives the ESM asset base from `new URL('.', import.meta.url).pathname`, which is percent-encoded. The Windows runner's temp dir is an 8.3 short name (`RUNNER~1`), so `~` became `%7E`. This is **not** platform-specific — paths containing spaces break identically everywhere: ``` $ ncc build input.mjs -o '/tmp/out dir' Cannot find module '/tmp/out%20dir/runtime.json' ``` That expression is byte-identical in 1.7.3 and 1.10.2, so the bug is pre-existing. It was simply unreachable for this fixture before: 1.7.3 had no `node:` specifier support, so `import { resolve } from "node:path"` was not statically analyzable and `runtime.json` was never relocated. 1.8.0 added `node:path` as an alias of `path`, so the reference is now resolved, emitted as an asset, and rewritten to `__nccwpck_require__.ab + ...` — which is where the latent bug surfaces. This PR originally carried the one-line fix (wrap the pathname in `decodeURIComponent`) as a `patchedDependencies` entry. **That patch has now shipped upstream in 1.10.3**, so this PR bumps to 1.10.3 and deletes: - `patches/@vercel__webpack-asset-relocator-loader@1.10.2.patch` - the matching `patchedDependencies` entry in `pnpm-workspace.yaml` (that file is now identical to `main` again) 1.10.3 is 1.10.2 plus exactly that change — `diff`ing the two published tarballs shows only the version field and the one `requireBase` line (plus an explanatory comment). Confirmed empirically: `update-fixtures.sh` regenerates every `test/unit/*/output*.js` with **zero drift** against the patched-1.10.2 fixtures already in this PR. ## Also fixed `update-fixtures.sh` invoked `node node_modules/.bin/jest` for the coverage pass. Under pnpm that path is a shell shim rather than a JS entry point, so the step crashed with `SyntaxError: missing ) after argument list` and never regenerated any `output-coverage.js`. Switched both invocations to `node_modules/jest/bin/jest.js`, matching the `test` script in `package.json`. That broken step is why two `output-coverage.js` fixtures had drifted on `main` before this PR (`import-meta-cjs` and `ts-json-resolve`). Their regenerated output here therefore contains one small change each that is unrelated to the relocator. The drift went unnoticed because `test/integration.test.js` calls `process.exit(0)` in `afterAll` under coverage, so `pnpm test-coverage` exits 0 even when `test/unit.test.js` fails. ## Testing Re-run on Linux / Node 22.22.2 after the bump to 1.10.3: - `pnpm test` — **121 passed, 1 failed**. The only failure is `binary-require.js` (`Cannot find module './hello.node'`), because `pnpm build-test-binary` needs a C++ toolchain that the machine used for this re-run does not have, so `test/integration/hello.node` was never produced. CI installs `node-gyp` and builds it, and this test passed on the previous push. - `jest --coverage --globals '{"coverage":true}' test/unit` — **28 passed** - `bash update-fixtures.sh` — regenerates all fixtures, `git status` clean afterwards - `pnpm install --frozen-lockfile` — succeeds with no config override The `%20`/`%7E` case is now covered end-to-end rather than by hand: running the `create-require-runtime` integration test with `TMPDIR="/tmp/tmp dir~short"` reproduces the Windows failure mode on Linux. It passes on 1.10.3, and to confirm the check is actually sensitive I reverted `decodeURIComponent` in the built loader and watched it fail with exactly the CI error: ``` Cannot find module '/tmp/tmp%20dir%7Eshort/<hash>/runtime.json' ``` Two pre-existing quirks I ran into and confirmed also reproduce on `main`, so they are **not** introduced here and are left alone: - Under `--runInBand`, if `test/watcher.test.js` runs before `test/unit.test.js` in the same process, a unit test fails with `TypeError: Cannot read properties of undefined (reading 'then')` inside webpack's `FileSystemInfo`. Reproduced at 1.7.3. - The stale coverage fixtures described above. ## One thing to flag 1.10.3 was published a few minutes before this update was pushed, and `pnpm-workspace.yaml` sets `minimumReleaseAge: 2880` (48h), so a fresh resolution of this version is blocked today. I generated the lockfile with a one-off `--config.minimumReleaseAge=0` and deliberately did **not** add a `minimumReleaseAgeExclude` entry, so the repo's supply-chain policy is unchanged. CI is unaffected: both install steps in `ci.yml` use `pnpm install --frozen-lockfile`, which skips resolution and installs 1.10.3 from the lockfile (verified locally — it succeeds with no override). Once 1.10.3 is 48h old the constraint stops mattering entirely — hold this PR until then if you'd rather the policy not be bypassed at all. ## Related - Fixes #1307 Confirmed this bump does address it. That `TypeError: Cannot mix BigInt and other types` originates in the relocator's static evaluator (`relocate-loader.js.cache.js`), whose `BinaryExpression` handler is unguarded in 1.7.3. 1.10.x wraps it in a `try`, with a comment naming that exact error, and adds the matching guard for the `UnaryExpression` BigInt case. --------- Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com> Co-authored-by: Steven <229881+styfle@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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