feat: port ncc bundling pipeline to rspack - #1304
Conversation
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Handle both 4-argument and 5-argument watch callback signatures so changed and removed file sets are forwarded correctly and callbackUndelayed receives file metadata. Co-authored-by: Cursor <cursoragent@cursor.com>
45bfde1 to
a764e3c
Compare
There was a problem hiding this comment.
- Each TS file rebuilds and type-checks the entire dependency graph + lib files. The old ts-loader kept a single shared Program/instance and type-checked once per build. This is a clear regression. TS modules are wrapped by uncacheable.js (this.cacheable(false)), so rspack's persistent cache can't amortize any of it — every build and every watch rebuild pays full cost. This cost applies to all TS builds, not just ones with errors.
- Duplicate diagnostics (correctness/UX). getPreEmitDiagnostics returns whole-program diagnostics, so one error in a shared module is reported once per importer. Verified through real ncc — a single shared.ts error printed 3× (once per program that includes it). ts-loader on main de-duplicates via its shared instance.
- Fragile emit invariant. program.emit() is called with no target file (ts-loader.js:80), so it emits every file in the program; outputText keeps only the last writeFile. This happens to be correct because the root file is emitted last (I verified across chained and circular imports), but it's an undocumented invariant and it serializes every other module's output for nothing.
- Double module resolution in beforeResolve — src/index.js:246-298. For every request, the hook calls resolver.resolve(...) to test existence, then on success returns control so rspack resolves the same request again — i.e. two full resolves per resolved module. The old webpack resolver-plugin wrapped the single resolve call, so it resolved once. Scales with graph size (the integration builds still pass, so it's not catastrophic, but it's measurable overhead on large graphs). Hooking the failed-resolution path instead of pre-resolving everything would avoid the duplication.
|
@styfle I pushed 6aaddc2 with the follow-up fixes:
Local verification: For the resolver double-resolve point: I dug through Rspack's normal module factory path. I agree that the ideal shape is to handle this in the failed-resolution path rather than pre-resolving in The blocker is that Rspack does not appear to expose that failed-resolution recovery point to JS plugins today. So I left the existing I also refreshed checks: |
styfle
left a comment
There was a problem hiding this comment.
[
{
"file": "src/cli.js",
"line": 377,
"summary": "New .catch on the non-watch build path mishandles `ncc run` failures: it stringifies the rejection to '[object Object]', drops the child's real exit code, and violates
the silent flag.",
"failure_scenario": "`ncc run script.js` where script.js exits non-zero (e.g. throws) → handler's inner promise rejects with `{silent:true, exitCode:2}` → this catch runs
`stderr.write(err+'\\n')` printing '[object Object]' despite silent, then `process.exit(1)` losing exit code 2. In api mode it also overwrites `err.exitCode = 1`, so callers/tests see 1
instead of 2. Previously the rejection propagated to the top-level handler which honored `silent` and exited with `e.exitCode`."
},
{
"file": "src/index.js",
"line": 223,
"summary": "The beforeResolve notfound-probe resolves with the generic `getResolver(\"normal\")`, which does not carry the per-dependency `conditionNames` (import/require) that only
exist under `resolve.byDependency`.",
"failure_scenario": "An ESM build imports a package whose package.json exposes its entry only via `exports: { import: ... }` (no `main`/default). rspack's real esm resolver would
resolve it, but the probe's 'normal' resolver lacks the `import` condition, so the probe fails → `handleMissing` rewrites the request to `@@notfound.js?<req>` and rspack never attempts
its own (correct) resolution → the module becomes a runtime MODULE_NOT_FOUND."
},
{
"file": "src/index.js",
"line": 234,
"summary": "`isNotFoundError` classifies resolver failures purely by substring-matching the message ('NotFound' / \"Can't resolve\" / \"Cannot resolve\"); any other phrasing is
treated as a hard error.",
"failure_scenario": "If rspack's resolver returns an unresolvable-module error worded differently (e.g. 'Failed to resolve X' or a localized message), `isNotFoundError` returns false
and both probe paths call `callback(err)`, turning what ncc intends to be a runtime not-found (`__non_webpack_require__`) into a hard build failure — a regression of ncc's core 'make
not-found errors runtime errors' semantics."
},
{
"file": "src/index.js",
"line": 252,
"summary": "beforeResolve resolves every non-builtin/non-external request an extra time (probe) before rspack performs the real resolution, doubling resolver work for all modules on
all builds.",
"failure_scenario": "For a large dependency graph, each import triggers a full `resolver.resolve` in the probe (only to decide notfound), then rspack resolves the same request again
to actually build the module. The webpack implementation intercepted the single real resolution; the rspack workaround cannot, so resolution cost is ~2x on every build."
},
{
"file": "src/index.js",
"line": 513,
"summary": "Error-message assembly strips every line whose trimmed text begins with 'at ', so an error message composed solely of stack-like lines collapses to an empty string.",
"failure_scenario": "A compilation error whose `.message` is only stack frames (each line starting with 'at ') is filtered down to '', so `reject(new Error(errLog))` surfaces a blank
error and the user sees a failed build with no explanation."
},
{
"file": "src/loaders/ts-loader.js",
"line": 33,
"summary": "`stateKey = JSON.stringify(parsedOptions)` keys the shared type-check state on options resolved per-file via `convertCompilerOptionsFromJson(...,
path.dirname(fileName))`, so relative tsconfig paths yield a different key per source directory.",
"failure_scenario": "With a tsconfig using relative `baseUrl`/`paths`/`rootDir` and .ts files spread across directories, each directory produces distinct absolute-path options →
distinct stateKey → `getTypeCheckState` builds a separate full `ts.Program` per directory in `finishModules` instead of one, multiplying type-checking CPU across the build."
},
{
"file": "src/loaders/relocate-loader.js",
"line": 32,
"summary": "The `.json` branch in `wrappedRelocateLoader` is unreachable dead code and would bypass asset relocation if ever reached.",
"failure_scenario": "The module rule test `/\\.(js|mjs|tsx?|node)$/` excludes `.json`, and rspack parses JSON natively, so this loader is never invoked for `.json`. The branch adds
complexity for no effect; if a future rule change routed json here, it would return the file verbatim and skip the asset relocator entirely."
},
{
"file": "src/loaders/relocate-loader.js",
"line": 4,
"summary": "`ensureMainTemplate` installs a fake `compilation.mainTemplate.hooks.requireExtensions` whose tapped functions are never executed; the pinned relocator (1.10) never reads
`mainTemplate`.",
"failure_scenario": "`@vercel/webpack-asset-relocator-loader@^1.10.0` uses `runtimeRequirementInTree`/`addRuntimeModule`, not `mainTemplate` (verified: no reference in the package).
The shim collects taps into an array that is never invoked, so it is dead code that can mislead maintainers into thinking a `requireExtensions` hook is active."
}
]Rspack resolves in Rust and only reports the paths a resolver touched when a request resolves, so requests ncc rewrites to @@notfound.js left nothing for watch mode or the persistent cache to invalidate on. Derive the paths whose creation could satisfy a missed request and register them instead, stopping each candidate one segment past the deepest existing directory so rspack does not end up watching a far ancestor recursively. Also probe with the conditions of the dependency being resolved rather than a single default resolver, classify resolver misses from error codes instead of message substrings, keep compilation messages that look like stack frames, share one TypeScript program across source directories, and let ncc run child exit codes reach the CLI.
Track the bounded files that can satisfy rewritten resolver misses so watch mode and persistent cache recover when dependencies appear, including package mappings and TypeScript fallbacks. Restore syntax diagnostics, diagnostic deduplication, and declaration emission in the shared TypeScript program while removing an unreachable webpack parser shim.
|
Addressed styfle's latest requested changes and the follow-up parity audit in
Additional parity fixes:
Fresh verification on The unit suite also passes twice consecutively with the persistent cache warm. @styfle could you please re-review when you have a chance? The CI/PR workflows are waiting for maintainer approval. |
Preserve Node CommonJS retry semantics despite Rspack's strict-error option alias, and use a shared TypeScript Program for type-aware output, declarations, and diagnostics. Track discovered tsconfig files and update generated runtime fixtures to reflect the intentional cache-eviction behavior.
Route only error-category diagnostics to compilation failures while reporting warnings through the warning channels, matching ts-loader behavior without weakening syntax or config error handling.
Reuse package candidate and diagnostic reporting helpers, cache negative external lookups and filesystem metadata, and centralize watch-test plumbing without changing behavior.
Summary
Testing