fix(schematics): restore ng deploy under the CommonJS schematics bundle - #3729
fix(schematics): restore ng deploy under the CommonJS schematics bundle#3729armando-navarro wants to merge 2 commits into
Conversation
`ng deploy` threw at module load in 21.0.0-rc.0, before any user code ran:
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type
string or an instance of URL. Received undefined
at fileURLToPath (node:internal/url)
The schematics are bundled by esbuild with `format: "cjs"`, and esbuild
rewrites `import.meta` to an empty object in CommonJS output. The shipped
bundle therefore read `undefined.url`, so both `deploy/actions.js` and
`deploy/builder.js` failed to load. Every other shipped entry point (ng add,
both ng update migrations, the setup schematic) was unaffected.
The shim was introduced when `versions.json` moved from a compile-time import
to a runtime read. That move fixed a real bug of its own: because esbuild
bundles before the build copies and rewrites `versions.json`, the compile-time
import inlined the unreplaced `0.0.0` placeholders, and 20.0.1 generates a
Cloud Functions manifest pinning `0.0.0` that cannot install. So the runtime
read has to stay.
`typeof` on an undeclared identifier is the one form that does not throw under
ESM, so a single expression works under both loaders, and the CommonJS branch
comes first because `import.meta` is the substituted empty object there. The
alternatives were built and run, not assumed:
- plain `__dirname` breaks `npm run test:node-esm`, which genuinely loads
the compiled specs as ESM
- `require('../versions.json')` reintroduces the `0.0.0` bug above
- an esbuild define/banner works today but fails with "require is not
defined in ES module scope" the moment `format: "esm"` is enabled, which
tools/build.ts already has staged in a comment
Verified against the built package: all seven shipped entry points now load
via both `require()` and `await import()`, the builder exposes the Architect
builder symbols, and the runtime `versions.json` read resolves correctly.
`ng lint` also drops its only warning, which sat on the replaced line.
This is v21-only. v20 has no `import.meta` shim and must not take this change.
The load failure fixed in the previous commit reached a published release
because nothing in the build or the test suite ever loads what actually ships.
The jasmine suite runs against the TypeScript output, which is a different
module format from the CommonJS bundle in the package, so a bundle can be
completely unloadable while every test passes.
Requiring each compiled entry point at the end of the schematics build closes
that gap. Reverting the previous commit now fails the build with the real
error:
Compiled schematics failed to load:
deploy/actions.js: TypeError [ERR_INVALID_ARG_TYPE] ...
deploy/builder.js: TypeError [ERR_INVALID_ARG_TYPE] ...
It catches the whole class, not just this instance: an unresolvable import, a
bad top-level require, or anything else that throws at module load.
796ff8d to
5c7aba4
Compare
tyler-reitz
left a comment
There was a problem hiding this comment.
The mechanism is sound. typeof on a possibly-undeclared identifier is the one form that doesn't throw under ESM, and putting the CommonJS branch first is right given esbuild substitutes import.meta with an empty object rather than leaving it undefined — fileURLToPath(undefined) is exactly the reported crash.
Checks I ran:
__dirnamehad exactly one consumer insrc/schematics/(theversions.jsonread ingetPackageJson), so the rename tomoduleDirectorycovers every use.dest()returns an absolute path, sorequire(path)in the new check resolves as a file rather than a bare specifier.tools/build.tscompiles to CommonJS pertsconfig.build.json, sorequireis legitimately in scope there — consistent with its existing use inreplacePackageCoreVersion.
Appreciate the second commit especially. A test suite that runs against a different module format than the one you publish will keep letting this class through, and requiring the real artifact is the cheapest possible fix.
One nit, non-blocking: loadCompiledSchematics() hardcodes a second copy of the seven entry points that compileSchematics() already lists in its esbuild config. A new entry point added to the esbuild list would silently skip the load check — the exact failure mode this PR exists to prevent. Worth hoisting to a single array and mapping .ts to .js for the check.
ng deployfails immediately in 21.0.0-rc.0. The command throws before it does any work:Cause
deploy/actions.tsderived its own directory withfileURLToPath(import.meta.url). The schematics ship as a CommonJS bundle, and esbuild rewritesimport.metato an empty object in CommonJS output, so the shipped code calledfileURLToPath(undefined). Bothdeploy/actions.jsanddeploy/builder.jsfailed to load as a result.The shim arrived when
versions.jsonmoved from a compile-time import to a runtime read. That move fixed a real problem of its own: esbuild bundles before the build copies and rewritesversions.json, so the compile-time import inlined the unreplaced0.0.0placeholders, and 20.0.1 generates a Cloud Functions manifest pinning0.0.0, which cannot install. The runtime read needs to stay.The change
typeofon an undeclared identifier is the one form that does not throw under ESM, so a single expression covers both module formats:The CommonJS branch is deliberately first, since
import.metais the substituted empty object in that bundle.I built and ran the alternatives rather than reasoning about them:
__dirnamealone breaksnpm run test:node-esm, which genuinely loads the compiled specs as ES modules.require('../versions.json')reintroduces the0.0.0problem described above.defineor banner works today, but fails withrequire is not defined in ES module scopethe momentformat: "esm"is enabled, whichtools/build.tsalready has staged in a comment.Second commit: catching this class at build time
This reached a release because nothing in the build or the test suite loads what actually ships. The jasmine suite runs against the TypeScript output, a different module format from the published CommonJS bundle, so a bundle can be entirely unloadable while every test passes.
The build now requires each compiled entry point before publishing. Reverting the first commit fails the build with the real error:
Verification
require()andawait import(). Before this change, the two deploy bundles threw and the other five were fine.versions.jsonread resolves correctly and returns real versions.deployToFunction()runs to completion against mocks, writing both the generated manifest and function file.ng lintdrops its only warning, which sat on the replaced line.Scope
Version 21 only. Version 20 has no
import.metashim and must not take this change.This fixes loading, so hosting-only deploys work again. Deploying SSR to Cloud Functions is separately broken and not addressed here.