-
Notifications
You must be signed in to change notification settings - Fork 13.3k
fix(angular): restore change detection for routed pages #31414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0c1518a
fix(angular): restore change detection for routed pages
ShaneK 7c47c4f
Merge branch 'main' of github.com:ionic-team/ionic-framework into FW-…
ShaneK 0739c1b
chore(angular): harden change detection build checks and document them
ShaneK a20dc3a
fix(angular): preserve source maps and stabilize change detection tests
ShaneK efe166d
test(angular): fix landing page navigation between test apps
ShaneK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # Angular Change Detection | ||
|
|
||
| Every `@Component` in `packages/angular/src` must declare `changeDetection` explicitly. What the Angular partial linker fills in for an undeclared strategy depends on two versions: the one stamped into our own emitted declaration, and the linker the consumer runs. An Angular 22 linker fills in `OnPush` when our declaration says 22 or later, while Angular 18-21 linkers always fill in `Default`. So bumping *our* toolchain to Angular 22 is enough to flip every Angular 22 consumer, which is how `ion-router-outlet` and `ion-tabs` stopped letting change detection reach routed pages in [#31406](https://github.com/ionic-team/ionic-framework/issues/31406). | ||
|
|
||
| Use `OnPush` unless the pages a component hosts are only reached by a tick descending through its own view. Only `ion-router-outlet` and `ion-tabs` qualify today. The `ion-nav` component stays `OnPush` because `IonNavBase` detaches its view and the delegate attaches its pages as root views instead. | ||
|
|
||
| A component that needs `Default` also needs two things the compiler won't warn about: | ||
|
|
||
| - an `// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection` above it, with a comment saying why. That rule is an error here. It only fires on an explicit non-OnPush value, never on a missing one, so it can't enforce the rule above on its own. | ||
| - an entry in `EAGER_COMPONENTS` in `packages/angular/scripts/verify-change-detection.js`, keyed by class name and listing every dist file it is emitted into (one for lazy, one for standalone). | ||
|
|
||
| `IonRouterOutlet` carries both. In `packages/angular/src/standalone/navigation/router-outlet.ts`: | ||
|
|
||
| ```ts | ||
| @ProxyCmp({ | ||
| defineCustomElementFn: defineCustomElement, | ||
| }) | ||
| @Component({ | ||
| selector: 'ion-router-outlet', | ||
| standalone: true, | ||
| // Routed pages are created inside this component's own view, so an OnPush | ||
| // outlet would leave them unreachable from a tick under Zone.js. | ||
| // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection | ||
| changeDetection: ChangeDetectionStrategy.Default, | ||
| template: '<ng-container #outletContent><ng-content></ng-content></ng-container>', | ||
| }) | ||
| export class IonRouterOutlet extends IonRouterOutletBase { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| The lazy build declares the same component at `packages/angular/src/lazy/directives/navigation/ion-router-outlet.ts`, with the same comment, eslint-disable and `changeDetection` line. It sets `standalone: false` and has no `@ProxyCmp`. | ||
|
|
||
| And in `packages/angular/scripts/verify-change-detection.js`: | ||
|
|
||
| ```js | ||
| const EAGER_COMPONENTS = { | ||
| IonRouterOutlet: ['lazy/directives/navigation/ion-router-outlet.js', 'standalone/navigation/router-outlet.js'], | ||
| IonTabs: ['lazy/directives/navigation/ion-tabs.js', 'standalone/navigation/tabs.js'], | ||
| }; | ||
| ``` | ||
|
|
||
| Everything else takes the plain form: `changeDetection: ChangeDetectionStrategy.OnPush`, no eslint-disable, no script entry. See `packages/angular/src/standalone/navigation/nav.ts`, where the comment records why `ion-nav` stays `OnPush` despite hosting pages, and `packages/angular/src/lazy/directives/navigation/ion-nav.ts`, with the same comment. | ||
|
|
||
| The `npm run build` script enforces this in two steps: | ||
|
|
||
| - The `build.change-detection` step (`packages/angular/scripts/normalize-change-detection.js`) rewrites Angular 22's emitted `ChangeDetectionStrategy.Eager` back to `Default`, since `Eager` only exists from Angular 21.2 onward and earlier linkers in the peer range reject it outright. | ||
| - The `validate.change-detection` step (`packages/angular/scripts/verify-change-detection.js`) fails the build on a component with no strategy, a strategy name that won't link across the whole peer range, or a component going eager without being listed in `EAGER_COMPONENTS`. | ||
|
|
||
| If that check names a component from `packages/angular/src/lazy/directives/proxies.ts` or `packages/angular/src/standalone/directives/ion-*.ts`, don't edit those files. They are emitted by `@stencil/angular-output-target`, which hardcodes the strategy, so a failure there means the generator changed. Fix or pin that dependency in `core/package.json` instead. Those generated files are 158 of the 182 components and `packages/angular/eslint.config.js` ignores all of them, so lint can never see them, which is why this check exists. | ||
|
|
||
| ## Other version-gated linker behavior | ||
|
|
||
| The change detection default is one of two linker gates that the Angular 22 bump flipped. The other is `legacyOptionalChaining`: on a declaration stamped 22 or later, `a?.b` on a nullish receiver evaluates to `undefined` per the JS spec, while an Angular 18-21 linker keeps the legacy `null`. No template or host binding in `packages/angular/src` uses `?.` today, so nothing is affected, but the first one added becomes version-dependent with no test that notices. | ||
|
|
||
| Both gates key off the same `version:` string the compiler stamps into our emitted declarations, so a future toolchain bump can change emitted output the same silent way. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const { SourceMapConsumer, SourceMapGenerator } = require('source-map'); | ||
|
|
||
| /** | ||
| * Rewrite `ChangeDetectionStrategy.Eager` to `Default` in the compiled output. | ||
| * | ||
| * Angular 22 renamed `Default` to `Eager`. Both names are the same value, but a | ||
| * linker that doesn't know the new one fails the consumer's build on it. The | ||
| * rename was backported to 21.2, so remove this once the peer range's lowest | ||
| * version is 21.2 or higher. 21.0 and 21.1 still reject it. | ||
| */ | ||
|
|
||
| const DIST_DIR = path.join(__dirname, '../dist'); | ||
| const ANGULAR_22_NAME = 'ChangeDetectionStrategy.Eager'; | ||
| const PORTABLE_NAME = 'ChangeDetectionStrategy.Default'; | ||
|
|
||
| function listDistJsFiles() { | ||
| if (!fs.existsSync(DIST_DIR)) { | ||
| throw new Error('dist does not exist. build.ng emitted nothing.'); | ||
| } | ||
|
|
||
| // Any JS extension, so an emit that moves to .mjs isn't silently skipped here. | ||
| return fs | ||
| .readdirSync(DIST_DIR, { recursive: true }) | ||
| .filter((entry) => /\.(m|c)?js$/.test(entry)) | ||
| .map((entry) => path.join(DIST_DIR, entry)); | ||
| } | ||
|
|
||
| function rewriteSource(source) { | ||
| const replacements = new Map(); | ||
| const lines = source.split('\n'); | ||
|
|
||
| const rewritten = lines.map((line, index) => { | ||
| let searchFrom = 0; | ||
| let startColumn; | ||
|
|
||
| while ((startColumn = line.indexOf(ANGULAR_22_NAME, searchFrom)) !== -1) { | ||
| const lineNumber = index + 1; | ||
| const lineReplacements = replacements.get(lineNumber) ?? []; | ||
| lineReplacements.push({ | ||
| startColumn, | ||
| endColumn: startColumn + ANGULAR_22_NAME.length, | ||
| delta: PORTABLE_NAME.length - ANGULAR_22_NAME.length, | ||
| }); | ||
| replacements.set(lineNumber, lineReplacements); | ||
| searchFrom = startColumn + ANGULAR_22_NAME.length; | ||
| } | ||
|
|
||
| return line.split(ANGULAR_22_NAME).join(PORTABLE_NAME); | ||
| }); | ||
|
|
||
| return { rewritten: rewritten.join('\n'), replacements }; | ||
| } | ||
|
|
||
| function adjustGeneratedColumn(column, replacements) { | ||
| return replacements.reduce( | ||
| (adjusted, replacement) => (column >= replacement.endColumn ? adjusted + replacement.delta : adjusted), | ||
| column | ||
| ); | ||
| } | ||
|
|
||
| async function rewriteSourceMap(file, replacements) { | ||
| const mapFile = `${file}.map`; | ||
| if (!fs.existsSync(mapFile)) { | ||
| throw new Error(`Source map does not exist for ${path.relative(DIST_DIR, file)}.`); | ||
| } | ||
|
|
||
| const sourceMap = JSON.parse(fs.readFileSync(mapFile, 'utf8')); | ||
| const consumer = await new SourceMapConsumer(sourceMap); | ||
|
|
||
| try { | ||
| const generator = new SourceMapGenerator({ file: sourceMap.file, sourceRoot: sourceMap.sourceRoot }); | ||
| const originalSources = new Map( | ||
| consumer.sources.map((resolvedSource, index) => [resolvedSource, sourceMap.sources[index]]) | ||
| ); | ||
|
|
||
| consumer.eachMapping( | ||
| (mapping) => { | ||
| const lineReplacements = replacements.get(mapping.generatedLine) ?? []; | ||
| const generated = { | ||
| line: mapping.generatedLine, | ||
| column: adjustGeneratedColumn(mapping.generatedColumn, lineReplacements), | ||
| }; | ||
|
|
||
| if (mapping.source === null) { | ||
| generator.addMapping({ generated }); | ||
| } else { | ||
| generator.addMapping({ | ||
| generated, | ||
| source: originalSources.get(mapping.source) ?? mapping.source, | ||
| original: { line: mapping.originalLine, column: mapping.originalColumn }, | ||
| name: mapping.name ?? undefined, | ||
| }); | ||
| } | ||
| }, | ||
| null, | ||
| SourceMapConsumer.GENERATED_ORDER | ||
| ); | ||
|
|
||
| for (const source of consumer.sources) { | ||
| const content = consumer.sourceContentFor(source, true); | ||
| if (content !== null) generator.setSourceContent(source, content); | ||
| } | ||
|
|
||
| const rewrittenMap = JSON.parse(generator.toString()); | ||
| for (const [key, value] of Object.entries(sourceMap)) { | ||
| if (!Object.hasOwn(rewrittenMap, key)) rewrittenMap[key] = value; | ||
| } | ||
| fs.writeFileSync(mapFile, JSON.stringify(rewrittenMap)); | ||
| } finally { | ||
| consumer.destroy(); | ||
| } | ||
| } | ||
|
|
||
| async function normalize() { | ||
| const files = listDistJsFiles(); | ||
| let rewritten = 0; | ||
|
|
||
| for (const file of files) { | ||
| const source = fs.readFileSync(file, 'utf8'); | ||
| if (!source.includes(ANGULAR_22_NAME)) continue; | ||
|
|
||
| const normalized = rewriteSource(source); | ||
| await rewriteSourceMap(file, normalized.replacements); | ||
| fs.writeFileSync(file, normalized.rewritten); | ||
| rewritten++; | ||
| } | ||
|
|
||
| console.log(`✅ normalized change detection strategy in ${rewritten} file(s)`); | ||
| } | ||
|
|
||
| module.exports = { DIST_DIR, PORTABLE_NAME, listDistJsFiles }; | ||
|
|
||
| if (require.main === module) { | ||
| normalize().catch((error) => { | ||
| console.error(error); | ||
| process.exitCode = 1; | ||
| }); | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.