Skip to content

Nbind binary support - #220

Merged
rauchg merged 1 commit into
masterfrom
nbind
Jan 15, 2019
Merged

rauchg merged 1 commit into
masterfrom
nbind

Conversation

@guybedford

Copy link
Copy Markdown
Contributor

This provides nbind binary asset emission support (just like we do for node-gyp and bindings).

Fixes #132 supporting yoga-layout.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #220 into master will increase coverage by 1.07%.
The diff coverage is 88.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   68.84%   69.92%   +1.07%     
==========================================
  Files          13       13              
  Lines         597      635      +38     
==========================================
+ Hits          411      444      +33     
- Misses        186      191       +5
Impacted Files Coverage Δ
src/cli.js 0% <ø> (ø) ⬆️
src/loaders/relocate-loader.js 92.33% <88.63%> (-0.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe1099f...aec49aa. Read the comment docs.

@rauchg
rauchg merged commit 1987382 into master Jan 15, 2019
@rauchg
rauchg deleted the nbind branch January 15, 2019 17:06
styfle added a commit 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>
Sign up for free to 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.

3 participants