Repository navigation
feat(build)!: compile with the TypeScript 7 native compiler - #11793
raymondfeng wants to merge 9 commits into
Conversation
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.
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
|
@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>
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