Visitar URL original
fix: support the packaging format of sharp v0.34.x by yjoer · Pull Request #199 · vercel/webpack-asset-relocator-loader · GitHub
Skip to content

fix: support the packaging format of sharp v0.34.x - #199

Merged
styfle merged 7 commits into
vercel:mainfrom
yjoer:patch-3
Jul 10, 2025
Merged

styfle merged 7 commits into
vercel:mainfrom
yjoer:patch-3

Conversation

@yjoer

@yjoer yjoer commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

The idea is stolen from nft, where we introspect package.json to retrieve optional dependencies containing the native addon and relocate them. Following that, I patched the third item within the path list, which will be used to load the addon file in the subsequent lines.

Since the prebuilt binaries are available on all platforms I could name, we may ignore the WASM builds. I've added integration tests requiring actual packages, but haven't updated the CI workflow yet, as it would cause longer CI times. The tests include sharp v0.34.2 and v0.32.6 to ensure the new mechanism doesn't affect the existing workflow. I've also run the scripts on both Windows and Linux (x64), and they worked as expected.

Another caveat is on the loading of libvips. On Windows, the dynamic libraries are included in a single package. While for Linux and Mac, they are divided into two packages. From a quick search, it seemed that they are only needed to build the binding. Currently, they are copied, but nothing in the source is changed to load them.

// windows
sharp
└─ @img/sharp-win32-x64

// linux
├─ @img/sharp-libvips-linux-x64
├─ @img/sharp-libvips-linuxmusl-x64
├─ @img/sharp-linux-x64
└─ @img/sharp-linuxmusl-x64

// mac
├─ @img/sharp-darwin-arm64
└─ @img/sharp-libvips-darwin-arm64

Fixes: vercel/ncc#1153

@yjoer
yjoer requested review from Timer and styfle as code owners July 4, 2025 18:33
@socket-security

socket-security Bot commented Jul 4, 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
Addedsharp@​0.32.6981001008970
Addedsharp@​0.34.29210010089100

View full report

@yjoer

yjoer commented Jul 4, 2025

Copy link
Copy Markdown
Contributor Author

On libvips, I tried deleting the sharp-libvips-linux-x64 folder, and the terminal screams at me.

ERR_DLOPEN_FAILED: libvips-cpp.so.8.16.1: cannot open shared object file: No such file or directory

It is good when the folders are colocated, which is already the case.

assets
├─ @img/sharp-libvips-linux-x64
└─ @img/sharp-linux-x64

@yjoer

yjoer commented Jul 8, 2025

Copy link
Copy Markdown
Contributor Author

Should I include the packages used in the integration test in dev dependencies instead? 🤔

@styfle

styfle commented Jul 9, 2025

Copy link
Copy Markdown
Member

Yeah I think we generally add the deps for tests into devDependencies

@styfle

styfle commented Jul 9, 2025

Copy link
Copy Markdown
Member

If we don't have any existing tests like this, you could update the CI command to install the subdirectory too

@yjoer

yjoer commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

This is ready for review. All nested optional dependencies are already included within the optional dependencies of the main package. Relocating them separately isn't necessary. Leaving the implementation here in case I've missed any packages.

try {
  for (const innerDep of Object.keys(pkg.optionalDependencies || {})) {
    const innerDir = path.resolve(fs.realpathSync(dir), '..', '..', innerDep);
    emitAssetDirectory(innerDir);
  }
} catch (err) {
  if (err && err.code !== 'ENOENT') {
    console.error(`Error reading 'sharp' dependencies from '${dir}/package.json'`);
    throw err;
  }
}


const fs = require('fs');

expect([26, 27]).toContain(output.length);

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.

Any reason this assertion for output length was removed?

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.

I changed the way webpack is invoked in d7ee3a0 so that the executions within the compilation are propagated to the coverage report. The output string from the standard output is replaced with a stats object. I am not aware of a way to retrieve a string like previous stdout from that object, so I replaced the assertion by checking the number of errors. The object provides a list of assets emitted, too, but the number of assets varies between platforms, making it suboptimal for assertion.

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.

In case the compilation fails, the promise is thrown, and the test fails. There isn't a try-catch block attempting to recover from or hide failures.

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.

Oh, right. I can assert the number of chunks, since it remains stable unless the entry point changes or dynamic imports are added. Matching the list of emitted assets with regexes for a set of expected files works as well.

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.

Added in 0093631

@styfle styfle changed the title feat: support the packaging format of sharp v0.34.x fix: support the packaging format of sharp v0.34.x Jul 10, 2025
@styfle
styfle merged commit 2efe28c into vercel:main Jul 10, 2025
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@yjoer
yjoer deleted the patch-3 branch July 10, 2025 17:49
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>
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.

BUG: Fails to bundle sharp@0.33.1

2 participants