feat(build)!: compile with the TypeScript 7 native compiler - #11793
raymondfeng wants to merge 9 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
samarpanB
left a comment
samarpanB
left a comment
There was a problem hiding this comment.
Thanks for this, the TS 6/7 investigation and the write-up are really thorough, and the build speedup is great.
My main concern is scope and semver. The PR bundles four independent changes: the TS 7 toolchain, typescript-eslint v8, ~60 ||→?? / ??= rewrites in framework code, and an unrelated CodeQL fix in the mock OAuth2 fixture. Two of them are breaking for downstream users but land under a non-breaking feat(build).
Blocking
@typescript/nativeis a harddependencyof@loopback/build, so the "fall back totypescriptwhen TS 7 isn't installed" path is never reached for consumers. Any app pinned to TS 5.x gets compiled by TS 7 on a minor bump (see inline). This needsfeat(build)!+BREAKING CHANGE:, or the native compiler should be opt-in.@loopback/eslint-config→ typescript-eslint v8 is also breaking for consumers (different peer ranges, new/renamed rules). It needs a major bump too.packages/cli/.yo-rc.jsonis regenerated with every generator'sargumentsemptied (see inline).
Suggest splitting out
- The
||→??conversions. Most are equivalent, but several change behavior when a value is''in core packages (inline examples incontext,core,boot; alsoextension-point.ts,inject-config.ts,controller-route.ts,build-schema.ts,repository.mixin.ts,lb3app.booter.tsrestApiRoot). They may all be improvements, but they deserve their ownrefactor/fixPR. Here, theprefer-nullish-coalescingoptions (ignorePrimitives,ignoreTernaryTests,ignoreIfStatements) could keep this PR behavior-neutral. - The
fix(mock-oauth2-provider)commit. The fix looks right, but it's unrelated to the TS upgrade.
Minor
try-catch-finally.unit.ts: threeeslint-disable-next-linecomments were replaced by blank lines. The lines can just be removed.graphql/keys.ts: a short comment on why the explicitBindingKey<...>annotation onGRAPHQL_CONTEXT_RESOLVERis needed would help.lb-ttsc/--use-ttypescriptis now effectively TS 6-only. Worth a README note or a deprecation.- The PR currently has merge conflicts with
master.
Looks good: the duplicate route() overload removal, the should-as-function move (nice catch on expect silently becoming any), the cron fireOnTick fix, the sync-dev-deps.js repair, and the test fixes in belongs-to / todo-list / transactions.suite.
| "@loopback/eslint-config": "^16.0.1", | ||
| "@types/mocha": "^10.0.10", | ||
| "@types/node": "^20.19.43", | ||
| "@typescript/native": "npm:typescript@~7.0.2", |
There was a problem hiding this comment.
Because this is a runtime dependency of @loopback/build, resolveNativeTsc() will always find it, via the __dirname entry in paths in compile-package.js. So the typescript/lib/tsc fallback described in the README/PR is unreachable for anyone who installs @loopback/build.
Before this PR, resolveCLI('typescript/lib/tsc') preferred the project's own typescript. Now an app pinned to TS 5.x, or one with "moduleResolution": "node" or extra global @types/* in its own tsconfig, is silently compiled by TS 7 and breaks on a minor version bump.
Either mark this as breaking (feat(build)! + BREAKING CHANGE: footer) or make @typescript/native opt-in (optional/peer dependency) so the fallback actually applies.
There was a problem hiding this comment.
Went with marking it breaking. The commit is now feat(build)! with a BREAKING CHANGE: footer, and it only touches @loopback/build, so lerna major-bumps that package alone. @typescript/native stays a hard dependency, and the unreachable typescript/lib/tsc fallback is removed.
| function resolveNativeTsc() { | ||
| try { | ||
| const manifest = require.resolve('@typescript/native/package.json', { | ||
| paths: [utils.getPackageDir(), __dirname], |
There was a problem hiding this comment.
Related to the package.json comment: __dirname here resolves @loopback/build's own copy of @typescript/native, which is always installed. If the fallback is meant to be reachable, only the project's getPackageDir() should be searched, or the dependency needs to be optional.
There was a problem hiding this comment.
The fallback is removed. resolveNativeTsc() still searches getPackageDir() before __dirname, so a project can pin its own @typescript/native; otherwise the copy @loopback/build depends on is used. README updated to match.
| "dependencies": { | ||
| "@typescript-eslint/eslint-plugin": "^7.18.0", | ||
| "@typescript-eslint/parser": "^7.18.0", | ||
| "@typescript-eslint/eslint-plugin": "^8.70.0", |
There was a problem hiding this comment.
typescript-eslint v7 → v8 is a breaking change for consumers of @loopback/eslint-config (peer ranges for typescript/eslint, renamed/split rules, and stricter prefer-nullish-coalescing defaults that will start flagging their code). I think this needs a major bump and a BREAKING CHANGE: note, ideally in its own commit.
There was a problem hiding this comment.
Split into #11811 as feat(eslint-config)! with a BREAKING CHANGE: footer. The lint fixes are in separate commits, so only @loopback/eslint-config gets the major bump.
| "types": ["node", "mocha"], | ||
|
|
||
| "lib": ["es2020"], | ||
| "module": "commonjs", |
There was a problem hiding this comment.
Nit: with moduleResolution removed, TS 6/7 default to Bundler. I checked that with module: commonjs both resolve in CJS mode with the require/types conditions, so it behaves correctly. Since every downstream app inherits this file, though, I'd set "moduleResolution": "bundler" explicitly (or mention the default in the README section) rather than rely on an implicit default.
There was a problem hiding this comment.
Set explicitly: "moduleResolution": "bundler", with a comment on the require/types conditions under module: commonjs. The README is updated too. Since @loopback/build now requires TypeScript 6+, the TS 5 incompatibility of bundler + commonjs no longer matters.
| "name": "name" | ||
| } | ||
| ], | ||
| "arguments": [], |
There was a problem hiding this comment.
This regeneration empties arguments for every generator. The cause is an existing bug in packages/cli/lib/cli.js: if (!gen) { for (const arg of gen._arguments) ... } never runs.
Tab completion only reads options, so users won't notice. But test/integration/cli/cli.integration.js ("saves command metadata to .yo-rc.json") snapshots this file, and the snapshot still has the full arguments, so that test now fails at the snapshot step instead of the "up to date" step. Since the PR notes this file is stale regardless of the change, I'd revert it here, and separately fix if (!gen) → if (gen) and regenerate the file and snapshot together.
There was a problem hiding this comment.
Reverted here. The if (!gen) bug is fixed in #11812, which also brings the stale snapshot up to date. The committed .yo-rc.json turned out to be correct; with the fix, lb4 --meta reports it as up to date.
| } | ||
| const user = users[0]; | ||
| if (!user.credentials || user.credentials.password !== password) { | ||
| if (user.credentials?.password !== password) { |
There was a problem hiding this comment.
This weakens the check slightly: if user.credentials is missing and password is undefined, undefined !== undefined is false and authentication passes. Passport normally hands over a string, so it's likely not reachable today, but this is example auth code people copy. I'd keep the explicit !user.credentials || guard (and disable the lint rule on this line if needed).
There was a problem hiding this comment.
Restored !user.credentials || with a scoped eslint-disable-next-line @typescript-eslint/prefer-optional-chain and a comment explaining why (in #11811).
| } | ||
| const user = users[0]; | ||
| if (!user.credentials || user.credentials.password !== password) { | ||
| if (user.credentials?.password !== password) { |
There was a problem hiding this comment.
Same concern as in basic.ts: please keep the explicit !user.credentials guard in auth code.
There was a problem hiding this comment.
Same as basic.ts: guard restored (in #11811).
| code: authCode, | ||
| }); | ||
| // redirect to call back url with the access code | ||
| const callbackUrl = parseRedirectUri(req.body.redirect_uri); |
There was a problem hiding this comment.
Small ordering point: the token is stored before redirect_uri is validated, so a rejected (400) request still leaves an issued token in tokens/issuedTokens. Validating redirect_uri first would avoid that. Also, since this commit is unrelated to the TS upgrade, it might be easier to review and land as its own PR.
| "debug": "^4.4.3", | ||
| "fs-extra": "^11.4.0", | ||
| "mocha": "^11.8.0", | ||
| "mocha": "^12.0.1", |
There was a problem hiding this comment.
The template mocha bump from ^11.8.0 to ^12.0.1 looks unrelated to the TS upgrade. Was it picked up by update-template-deps? Worth calling out in the description, or dropping.
There was a problem hiding this comment.
It came from update-template-deps: master already uses mocha ^12.0.2 (renovate), and the template was still on ^11.8.0. This is now noted in the feat(cli) commit message and the PR description.
| result.host = undefined; | ||
| } | ||
| // Set it to '' so that the http server will listen on all interfaces | ||
| result.host ??= undefined; |
There was a problem hiding this comment.
Nit: result.host ??= undefined only turns null into undefined, and the comment above still says "Set it to ''". Either note that the null → undefined conversion is intended or drop the line and its comment. The same pattern appears in extensions/socketio/src/socketio.server.ts.
There was a problem hiding this comment.
The comment now says: "Normalize a null host to undefined so that the http server listens on all interfaces". Updated in both rest.server.ts and socketio.server.ts.
typescript-eslint v7 does not support TypeScript 6 or later, so it blocks the TypeScript upgrade. Rules that v8 renamed or split are disabled to keep the rule set this configuration already had: `no-require-imports` replaces `no-var-requires`, and `ban-types` is split into `no-empty-object-type`, `no-unsafe-function-type` and `no-wrapper-object-types`. `prefer-nullish-coalescing` now ignores primitive operands. The rule runs without `strictNullChecks`, so it cannot tell whether a `string`, `number` or `boolean` operand may be nullish, and v8 reports `||` on such operands, where switching to `??` changes the result for `''`, `0` and `false`. BREAKING CHANGE: `@loopback/eslint-config` requires `@typescript-eslint/parser` and `@typescript-eslint/eslint-plugin` v8. Projects using it can get new findings, such as unused `catch` bindings (`no-unused-vars`), `prefer-optional-chain`, and `prefer-nullish-coalescing` for `if (x == null) x = y`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
- Drop unused `catch` bindings (`no-unused-vars`). - Use optional chains where they are equivalent (`prefer-optional-chain`). - Use `??=` for `if (x == null) x = y` and for `!x` guards on values that are objects or arrays, where it is equivalent (`prefer-nullish-coalescing`). - Remove the `route()` overload that `RestApplication` declared twice (`unified-signatures`). - Remove `eslint-disable` directives that no longer suppress anything. `||` is kept wherever the left operand may be `''` or `0`. The passport-login example keeps its explicit `!user.credentials` guard, since `user.credentials?.password !== password` accepts a user without credentials when `password` is `undefined`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
…n redirect Registered apps and issued tokens were held in plain objects keyed by values taken from the request, so a `__proto__` key reached `Object.prototype`. They are `Map`s now, which also drops the `[key: string]: any` index signature from the `App` interface (CodeQL js/prototype-polluting-assignment). `redirect_uri` is validated before a token is issued and the callback url is built from the parsed `URL` rather than by concatenating the request value. A real authorization server matches `redirect_uri` against the callback urls registered for the client; this provider only ever serves test applications running on the same machine, so it accepts loopback hosts (CodeQL js/server-side-unvalidated-url-redirection). The async route handlers are wrapped so that rejections reach Express instead of becoming unhandled promise rejections. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Install TypeScript side by side, as recommended for TypeScript 7: `typescript` is aliased to `@typescript/typescript6` so that tools consuming the JS compiler API keep working, and `@typescript/native` provides the native compiler that `lb-tsc` runs. A cold build of the monorepo drops from ~10.7s to ~2.1s. `lb-tsc` prefers the `@typescript/native` installed by the project being built over the one `@loopback/build` depends on. `lb-ttsc` keeps using the JS compiler, since `ttypescript` patches the compiler API. `tsconfig.common.json` changes for TypeScript 6/7: - `moduleResolution: node` (node10) was removed. The config sets `moduleResolution: bundler`, the TypeScript 6 default, which resolves `exports` maps with the `require` and `types` conditions for `module: commonjs`. - `@types/*` packages are no longer included automatically, so the config lists `types: ["node", "mocha"]`. BREAKING CHANGE: `@loopback/build` depends on TypeScript 7 (`@typescript/native`) and compiles with it, and its `typescript` dependency is TypeScript 6. Projects extending `@loopback/build/config/tsconfig.common.json` have to remove `moduleResolution: node`, and list ambient type packages other than `node` and `mocha` in `types`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Generated projects depend on TypeScript 7 (`@typescript/native`) and on TypeScript 6 as `typescript`, and no longer set `moduleResolution: node`, which TypeScript 7 removed. The template's `mocha` moves to `^12` as well. `update-template-deps` syncs it with the monorepo, which already uses mocha 12. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Packages depend on TypeScript 6 as `typescript`, matching `@loopback/build`. Source changes required by TypeScript 6/7: - The default resolver for `module: commonjs` honors `exports` maps, so `@loopback/graphql` no longer imports `Middleware` from a deep `type-graphql` path and declares it instead. `GRAPHQL_CONTEXT_RESOLVER` gets an explicit type, since its inferred type is not portable (TS2883). - Reading a parent class field through `super` is an error (TS2855), so `@loopback/cron` calls `fireOnTick` through `BaseCronJob.prototype`. - Declaration emit no longer adds `/// <reference path>` for ambient files, so `@loopback/testlab` exports the should.js types from a module rather than a global declaration file. Without it, `expect` resolves to `any` in every consuming package and api-extractor cannot follow the `Internal` symbol. With TypeScript 6 type information, typescript-eslint reports more findings: - Async Express handlers in `@loopback/rest` and the rpc-server example pass rejections to `next` (`no-misused-promises`). - `??` / `??=` replace `||` and `!x` guards on objects and arrays (`prefer-nullish-coalescing`). `||` is kept where the operand may be `''`. - `transactions.suite.ts` awaits `disconnect()` (`no-floating-promises`). Test defects surfaced by the stricter compiler are fixed as well: `belongs-to-repository-factory.unit.ts` never assigned `companyRepo` and clobbered `customerRepo` instead, and the todo-list repository tests used repositories they never created. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
8915bc4 to
f25e352
Compare
raymondfeng
changed the title
|
@samarpanB this PR is now split per your review:
Minor items:
|
The US Census geocoder now returns slightly different coordinates for the test address, so `GeoLookupService` and `TodoApplication` tests fail on every platform, on master as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
The hook that launches puppeteer and loads the page intermittently exceeds 15 seconds on the ubuntu-latest runners. It now gets 30 seconds, the same as the hook that generates the bundle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
This branch has not been deployed
4 participants
Upgrades the monorepo to TypeScript 7, using the side-by-side install recommended for TypeScript 7:
TypeScript 7 no longer ships the JS compiler API: its
exports["."]resolves tolib/version.cjs. So anything that introspects the compiler (@typescript-eslint, editor language services,tsserver) needs the API that@typescript/typescript6still provides. The two packages declare different bin names (tscvstsc6), so they don't collide.packages/build/package.jsonis the source of truth.bin/sync-dev-depspropagates the spec to every workspace package, andbin/update-template-depscarries it into thelb4scaffolding templates.A cold build of the monorepo drops from ~10.7s to ~2.1s.
Commits
feat(build)!: the toolchain,tsconfig.common.json, and the README. This is the only commit withBREAKING CHANGE:, so lerna's conventional-commit bump makes a major release of@loopback/buildonly.@typescript/nativeis a hard dependency of@loopback/build, andlb-tscalways compiles with TypeScript 7. It prefers the project's own@typescript/nativeover the one@loopback/builddepends on. The fallback totypescript/lib/tscwas unreachable and has been removed.bin/sync-dev-deps.jshad been broken since the Lerna → npm workspaces migration (d5c4994 removedloadLernaRepo). It's repaired using the@npmcli/map-workspacespattern already used byupdate-template-deps.js.feat(cli): generated projects target TypeScript 7 and dropmoduleResolution: node. The template'smochamoves to^12;update-template-depssyncs it with the monorepo, which already uses mocha 12.fix:: the source changes TypeScript 6/7 requires, and thetypescriptalias in every package.TypeScript 6/7 changes that required source updates
moduleResolution: node(node10) was removed.tsconfig.common.jsonnow sets"moduleResolution": "bundler"explicitly, which is the TypeScript 6 default. Withmodule: commonjsit resolvesexportsmaps using therequireandtypesconditions. Because the resolver honorsexports,@loopback/graphqlno longer importsMiddlewarefrom a deeptype-graphqlpath (that path isn't exported) and declares the type itself.@types/*packages are no longer auto-included. This accounted for 5630 of the 5668 initial errors (describe,process,NodeJSall unresolved).tsconfig.common.jsonnow liststypes: ["node", "mocha"].superis an error (TS2855).crondeclaresfireOnTickas a field but installs it on the prototype, so@loopback/cronnow calls it throughBaseCronJob.prototype./// <reference path>for ambient files. TypeScript 5.2 emitted one intotestlab/dist/expect.d.ts, which is how consumers resolved the globalInternaltype. Without it,expectsilently becomesanyin every consuming package, and api-extractor fails withUnable to follow symbol for "Internal". Soshould-as-function.d.tsmoves tosrc/should-as-function.tsand exports its types as a module.GRAPHQL_CONTEXT_RESOLVERgets an explicit type. Otherwise declaration emit has to nameContextFunctionby its path inside@apollo/server.With TypeScript 6 type information, typescript-eslint reports more findings:
@loopback/restand the rpc-server example now pass rejections tonext.transactions.suite.tsawaitsdisconnect().??replaces||on objects and arrays.||is kept wherever the operand may be''(see feat(eslint-config)!: upgrade typescript-eslint to v8 #11811 for theignorePrimitivesrationale).Defects found along the way
belongs-to-repository-factory.unit.tsnever assignedcompanyRepo. Its stub helper assignedcustomerRepotwice, clobbering the previousbeforeEach.Verification
npm run lint(eslint + prettier)@loopback/buildtestsnpm run mochaThe 220 failures don't come from this change:
packages/cligenerator tests. They refuse to prompt when stdin isn't a TTY, so they fail in a non-interactive shell.Checklist
npm testpasses on your machine: see Verificationpackages/cliwere updatedexamples/*were updated🤖 Generated with Claude Code