chore: TypeScript 5.8, nx migrate, dep updates #10759
Conversation
WalkthroughThis update includes dependency upgrades across multiple package manifests and configuration formatting improvements. Several TypeScript error suppression comments were added to native property setters. Type annotations were adjusted for HTTP and XHR interfaces, and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTPModule
participant XHRModule
User->>HTTPModule: Send HTTP request with content (string | FormData | ArrayBuffer | Uint8Array<ArrayBufferLike>)
HTTPModule-->>User: Returns promise response
User->>XHRModule: Use FileReader to read data
XHRModule-->>User: result can be string, ArrayBuffer, or SharedArrayBuffer
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 081dfb9
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/devtools/tsconfig.json (1)
7-7: Same caveat as the root configThe concerns raised for the root
tsconfig.base.jsonapply here as well. Ensure any dev-tools-specific scripts or bundlers (vite/esbuild/rollup) are exercised with this setting.
🧹 Nitpick comments (2)
tools/scripts/build-finish.ts (1)
16-29: DisablingcleanPackage()may publish unexpected"module"/"type"fieldsCommenting out both the implementation and call means packages in
distwill now keep whatever"module"or"type"fields the build pipeline generated.
If older consumers still rely on CommonJSrequire()this is a potential breaking change.Consider either:
- Fully deleting the dead code and documenting the intentional change in the changelog, or
- Re-introducing the cleanup behind an opt-in CLI flag (e.g.
--keep-module) so the behaviour is explicit.-// cleanPackage(); +if (!process.env.NS_KEEP_MODULE) { + cleanPackage(); +}packages/core/xhr/index.ts (1)
435-437: Return type annotation removed fromarrayBuffer()methodThe explicit
Promise<ArrayBuffer>return type annotation was removed. While TypeScript can infer this type, explicit return types improve code clarity and documentation.Consider restoring the explicit return type for better documentation:
- public arrayBuffer() { + public arrayBuffer(): Promise<ArrayBuffer> { return Promise.resolve(this._buffer); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.gitignore(1 hunks)apps/automated/package.json(1 hunks)apps/toolbox/package.json(1 hunks)apps/ui/package.json(1 hunks)migrations.json(1 hunks)nx.json(4 hunks)package.json(1 hunks)packages/core/.eslintrc.json(1 hunks)packages/core/http/index.ts(6 hunks)packages/core/profiling/index.ts(1 hunks)packages/core/ui/core/properties/index.ts(5 hunks)packages/core/ui/core/view/index.android.ts(5 hunks)packages/core/ui/image/index.ios.ts(1 hunks)packages/core/ui/search-bar/index.android.ts(1 hunks)packages/core/ui/search-bar/index.ios.ts(4 hunks)packages/core/ui/styling/style/index.ts(1 hunks)packages/core/ui/text-base/index.android.ts(1 hunks)packages/core/xhr/index.ts(2 hunks)packages/devtools/tsconfig.json(1 hunks)packages/webpack5/package.json(1 hunks)tools/scripts/build-finish.ts(1 hunks)tsconfig.base.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/core/ui/search-bar/index.android.ts (4)
packages/core/ui/search-bar/index.ios.ts (4)
textProperty(165-167)textProperty(169-172)hintProperty(174-176)hintProperty(178-180)packages/core/ui/text-base/text-base-common.ts (1)
textProperty(226-230)packages/core/ui/text-base/index.d.ts (1)
textProperty(213-213)packages/core/ui/editable-text-base/index.android.ts (4)
textProperty(192-194)textProperty(195-202)hintProperty(441-443)hintProperty(444-447)
packages/core/ui/styling/style/index.ts (1)
packages/core/data/observable/index.ts (1)
Observable(97-497)
packages/core/ui/text-base/index.android.ts (6)
packages/core/ui/styling/style-properties.ts (4)
paddingTopProperty(556-571)paddingRightProperty(538-553)paddingBottomProperty(574-589)paddingLeftProperty(520-535)packages/core/ui/styling/style-properties.d.ts (4)
paddingTopProperty(100-100)paddingRightProperty(99-99)paddingBottomProperty(101-101)paddingLeftProperty(98-98)packages/core/ui/button/index.android.ts (8)
paddingTopProperty(126-128)paddingTopProperty(129-131)paddingRightProperty(133-135)paddingRightProperty(136-138)paddingBottomProperty(140-142)paddingBottomProperty(143-145)paddingLeftProperty(147-149)paddingLeftProperty(150-152)packages/core/ui/layouts/layout-base.android.ts (8)
paddingTopProperty(30-32)paddingTopProperty(33-35)paddingRightProperty(37-39)paddingRightProperty(40-42)paddingBottomProperty(44-46)paddingBottomProperty(47-49)paddingLeftProperty(51-53)paddingLeftProperty(54-56)packages/core/core-types/index.ts (2)
LengthType(35-35)LengthType(325-325)packages/core/css/parser.ts (1)
Length(13-13)
packages/core/http/index.ts (1)
packages/core/xhr/index.ts (1)
FormData(356-376)
packages/core/ui/core/properties/index.ts (1)
packages/core/ui/styling/style/index.ts (1)
Style(37-261)
🪛 Biome (1.9.4)
packages/core/xhr/index.ts
[error] 502-502: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 502-502: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-ios
- GitHub Check: test-android
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (36)
tsconfig.base.json (1)
9-9: Check downstream tooling before locking in"bundler"resolution
moduleResolution: "bundler"is still relatively new (v5.7+) and differs from"node"in a few subtle but breaking ways (e.g. it ignorespathsat runtime, honours"exports"conditions, changes default extension guesses, etc.).
Please verify that:• Nx v21, Jest, ts-node, ESLint and any custom build scripts you use can all consume the generated output without additional flags.
• All internal import paths resolve correctly when built withtsc -b, especially those using the path aliases listed below in this file.If any tool in your CI/CD pipeline is still hard-wired to
"node"semantics you may need to keep a package-level override or introduce a compatibility flag.packages/core/profiling/index.ts (1)
113-121: Formatting-only change looks goodTernary compression doesn’t alter control flow; behaviour is preserved.
packages/webpack5/package.json (2)
20-54: Heads-up: multiple major-version bumps can introduce breaking API/ESM changesSeveral dependencies jump a major version (
babel-loader@10,css-loader@7,copy-webpack-plugin@13,sass-loader@16,webpack-cli@6,webpack-merge@6, …).
At least four of these packages switched to pure-ESM or changed option names (fork-ts-checker-webpack-plugindropped theasyncflag,webpack-mergeno longer accepts arrays, etc.). Please verify the build still succeeds and update webpack config where needed.
57-72: Confirm Angular 20 / TS 5.8 compatibility with plugin versions
@angular-devkit/build-angular@20andts-jest@29.4were added while TypeScript was bumped to 5.8. Make sure the compiled output targets identical TS minor and the jest preset still recognises.mts/.ctsfiles produced by Angular 20.apps/toolbox/package.json (1)
15-20: Platform packages bumped to 8.9 – SDK/CLI upgrade required
@nativescript/android|ios|visionos ~8.9.0require the matching NativeScript CLI ≥ 8.9 and, for iOS, Xcode 15 minimum. Double-check CI images before merging.apps/automated/package.json (1)
14-20: Verify automated-tests pipeline with TypeScript 5.8The automated app uses circular-dependency-plugin and custom webpack rules; ensure they compile cleanly against TS 5.8 and the new platform runtimes.
.gitignore (2)
61-64: Summary mismatch: capitalization change opposite to AI summaryThe diff shows
vitest.config.*.timestamp*added in lowercase, whereas the AI summary claims the line was capitalised toVitest. No problem in the code itself, but flagging the inconsistency for record.Likely an incorrect or invalid review comment.
63-64: Nice addition – ignore Nx rule & instruction artifacts
The new ignore patterns keep ancillary Nx metadata out of git.apps/ui/package.json (1)
14-19: Same platform/TS bumps – align e2e images
apps/uidrives e2e on real devices. Ensure the functional-tests-images repo already contains 8.9 runtimes; otherwise theload-imagesscript will fail.packages/core/ui/styling/style/index.ts (1)
37-37: Interface implementation removed as part of type system refinement.The removal of
StyleDefinitioninterface implementation aligns with the broader TypeScript 5.8 upgrade and type system simplification mentioned in the PR objectives. The class continues to extendObservablewhich provides the core functionality.packages/core/.eslintrc.json (1)
4-4: Good addition of ignore patterns for Vite/Vitest timestamp files.These ignore patterns correctly exclude auto-generated temporary files that don't need linting, which aligns with the Vite integration mentioned in the PR objectives.
packages/core/ui/search-bar/index.android.ts (2)
237-237: TypeScript error suppression added for native property setter.The
// @ts-expect-errorcomment is necessary due to stricter type checking in TypeScript 5.8 for property setters with symbol keys. This is part of a systematic effort across the codebase to handle similar typing issues.
245-245: TypeScript error suppression added for native property setter.The
// @ts-expect-errorcomment is necessary due to stricter type checking in TypeScript 5.8 for property setters with symbol keys. This is part of a systematic effort across the codebase to handle similar typing issues.packages/core/ui/image/index.ios.ts (1)
222-222: TypeScript error suppression added for native property setter.The
// @ts-expect-errorcomment is necessary due to stricter type checking in TypeScript 5.8 for property setters with symbol keys. This aligns with the systematic approach used across other UI components.packages/core/ui/search-bar/index.ios.ts (4)
6-6: Import refactored to use centralized constant.Good change to import
SDK_VERSIONfrom the constants module instead ofiOSNativeHelper, promoting better code organization and centralized constant management.
63-63: Version check updated to use centralized constant.The logic remains identical but now uses the centralized
SDK_VERSIONconstant, which is consistent with the import change and promotes better maintainability.
168-168: TypeScript error suppression added for native property setter.The
// @ts-expect-errorcomment is necessary due to stricter type checking in TypeScript 5.8 for property setters with symbol keys. This maintains consistency with the systematic approach used across the codebase.
177-177: TypeScript error suppression added for native property setter.The
// @ts-expect-errorcomment is necessary due to stricter type checking in TypeScript 5.8 for property setters with symbol keys. This maintains consistency with the systematic approach used across the codebase.packages/core/ui/core/view/index.android.ts (5)
836-844: Verify the necessity of TypeScript error suppression for accessibility role setter.The addition of
// @ts-expect-errorsuggests a type mismatch in the accessibility role property setter. Since this is part of the TypeScript 5.8 upgrade, please verify:
- What specific TypeScript error is being suppressed?
- Is this a temporary measure or a permanent solution?
- Could the underlying type issue be resolved instead of suppressed?
The systematic addition of these error suppressions across multiple native setters may indicate broader type system changes that should be addressed at the root cause level.
869-885: Verify the necessity of TypeScript error suppression for accessibility live region setter.Similar to the accessibility role setter, this error suppression should be verified to ensure it's not masking a real type safety issue.
887-891: Verify the necessity of TypeScript error suppression for accessibility state setter.This continues the pattern of error suppression for accessibility-related property setters. Please ensure these suppressions are justified and documented.
980-1016: Verify the necessity of TypeScript error suppression for horizontal alignment setter.The horizontal alignment property setter now has error suppression. Given that similar layout-related properties in other files (like
packages/core/ui/layouts/layout-base.android.ts) don't require error suppression, this warrants investigation.
1021-1057: Verify the necessity of TypeScript error suppression for vertical alignment setter.Similar concern as with the horizontal alignment setter. The selective application of error suppression suggests specific issues that should be understood and potentially resolved at the type level.
packages/core/ui/text-base/index.android.ts (2)
469-488: Consistent pattern of error suppression for all padding setters.All four padding property setters (top, right, bottom, left) follow the same pattern of error suppression. This reinforces the need to understand the root cause rather than treating symptoms.
Consider creating a type-safe solution that addresses the underlying TypeScript compatibility issue rather than suppressing errors across multiple methods.
461-464: Investigate TextBase padding property TypeScript suppressionsThe four
@ts-expect-errorlines on
[padding{Top,Right,Bottom,Left}Property.setNative]
in packages/core/ui/text-base/index.android.ts point to a missing or mismatchedsetNativesignature on theTextBaseclass (inherited fromTextBaseCommon). Identical code inbutton/index.android.tsandlayouts/layout-base.android.tscompiles cleanly, so this appears specific to TextBase’s type declarations.Please verify:
• The exact TypeScript errors these comments are silencing.
• WhetherpaddingXProperty.setNativeis declared onTextBaseCommon(or via theCssPropertydefinitions) so thatTextBaseactually implements the signature.
• If needed, update theCssProperty<Style, CoreTypes.LengthType>generics or augment theTextBaseCommoninterface (instead of suppressing) to properly type these setters.nx.json (3)
72-74: LGTM! Improved formatting for named inputs configuration.The conversion from multiline to single-line arrays improves readability while maintaining the same configuration values.
78-112: LGTM! Consistent formatting for target defaults.The formatting improvements across all target default input configurations enhance consistency and maintainability.
121-121: LGTM! Consistent formatting for release projects.The single-line format for the release projects array maintains consistency with the other formatting changes in this file.
migrations.json (1)
3-12: LGTM! Comprehensive migration configurations for Nx upgrade.The migration entries are properly configured with:
- Appropriate version targets (20.5.x, 21.0.x, 21.1.x)
- Clear descriptions of changes
- Correct package references and implementation paths
- Consistent naming conventions
These migrations align with the PR objectives of upgrading to TypeScript 5.8 and performing Nx migration.
packages/core/ui/core/properties/index.ts (2)
601-601: Class declaration simplified: removedimplements CssProperty<T, U>clauseThe
CssPropertyclass declaration was simplified by removing theimplements CssProperty<T, U>clause. This might resolve circular reference issues with TypeScript 5.8.This change appears to be a valid simplification that resolves potential TypeScript compilation issues while maintaining the class functionality.
199-199: Confirm necessity ofsetNativetype relaxationThe
setNativeproperty onProperty,CoercibleProperty,CssPropertyandCssAnimationProperty(packages/core/ui/core/properties/index.ts:199) has been changed fromsymboltoany. At runtime it’s still assigned viaSymbol(propertyName + ':setNative'), so the relaxation toanyonly weakens compile-time safety without an obvious benefit.• Declaration now:
public readonly setNative: any;
(formerlysymbol)
• Runtime assignment remains:
this.setNative = Symbol(propertyName + ':setNative');
• Dynamic usage throughout:
view[setNative](value)Consider restoring a more specific type and one of these approaches to preserve type safety:
- Keep
setNative: symboland add a symbol index signature to theViewinterface (e.g.interface View { [key: symbol]: any }) soview[setNative]()is valid in TS.- Keep
setNative: symboland cast on use:
(view as any)[setNative](value).- If
anyis truly required for TS 5.8 compatibility, please document the rationale in a code comment.Please verify whether relaxing
setNativetoanyis necessary or if we can revert tosymbolwith a safer indexing strategy.packages/core/xhr/index.ts (1)
500-500: Good addition:SharedArrayBuffersupport in FileReaderThe
FileReaderclass now supportsSharedArrayBufferin both the private_resultproperty and publicresultgetter. This enhances type coverage and aligns with modern JavaScript capabilities.This is a valuable enhancement that improves the FileReader API's type accuracy and supports SharedArrayBuffer operations.
Also applies to: 508-510
package.json (4)
93-93: TypeScript upgraded to 5.8.3TypeScript has been upgraded from ~5.6.0 to 5.8.3, which aligns with the PR objective of modernizing to TypeScript 5.8.
This upgrade enables access to the latest TypeScript features and improvements, supporting the modernization goals mentioned in the PR.
28-28: Nx ecosystem upgraded to version 21.xThe Nx packages have been consistently upgraded from 20.x to 21.2.1, including the main
nxpackage and all@nx/*scoped packages. The@nativescript/nxpackage was also updated to ^21.0.0.This coordinated upgrade ensures compatibility across the Nx ecosystem and provides access to the latest build and development tools.
Also applies to: 31-39, 75-75
46-47: Multiple development dependencies updatedSeveral key development dependencies have been updated to newer versions:
@typescript-eslintpackages: ^8.6.0 → ^8.35.0eslint-config-prettier: ~9.1.0 → ^10.0.0postcss-importandpostcss-loader: updated to ^16.0.0 and ^8.0.0ts-jest: 29.2.5 → 29.4.0viteandvitest: updated to ^6.0.0 and ^2.0.0These updates maintain compatibility with the TypeScript 5.8 upgrade and provide access to the latest tooling improvements. The version ranges appear appropriate for a stable build environment.
Also applies to: 62-62, 79-80, 87-87, 94-95
68-68: Confirm and configurejitiusage in the@nx/vitemigrationIt looks like you’ve added
jiti@2.4.2to support theinstall-jitimigration for parsing TS in PostCSS files—but I wasn’t able to find any direct imports or calls tojitiin the source. Please verify that:
- The migration implementation file (
install-jiti.[js|ts]) undersrc/migrations/update-20-5-0/exists and invokesjiti.- The runtime code in the
@nx/viteplugin properly imports or requiresjiti(e.g.import('jiti')()orrequire('jiti')).- Any Vite/bundler resolution settings include support for TS‐based PostCSS files via
jiti.You can use these commands to locate and inspect the implementation:
# Locate the migration and its implementation rg -l "install-jiti" -t js -t ts # Search for direct jiti imports or requires rg -l "from ['\"]jiti['\"]" -t js -t ts rg -l "require\(['\"]jiti['\"]\)" -t js -t ts
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Style