Visitar URL original
fix --watch abbreviation in cli usage by maxbeatty · Pull Request #195 · vercel/ncc · GitHub
Skip to content

fix --watch abbreviation in cli usage - #195

Merged
rauchg merged 1 commit into
vercel:masterfrom
maxbeatty:fix-cli-usage
Dec 26, 2018
Merged

rauchg merged 1 commit into
vercel:masterfrom
maxbeatty:fix-cli-usage

Conversation

@maxbeatty

Copy link
Copy Markdown
Contributor

little typo 😇

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #195 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #195   +/-   ##
=======================================
  Coverage   67.87%   67.87%           
=======================================
  Files          12       12           
  Lines         579      579           
=======================================
  Hits          393      393           
  Misses        186      186
Impacted Files Coverage Δ
src/cli.js 0% <ø> (ø) ⬆️

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 f62eb38...a71392a. Read the comment docs.

@rauchg
rauchg merged commit dba447a into vercel:master Dec 26, 2018
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