Enables the incremental type-checking test that we never enabled when we
landed signal inputs. Now that we fixed incremental re-use by re-using
the existing user imports for inline type check blocks, the test is
passing and can be enabled.
PR Close#54819
Updates the type-check block generation code (also for inline type check
blocks) to use the new import manager.
This is now a requirement because the translator utilities from the
reference emit environment expect an import manager that follows the
new contract established via `ImportGenerator<TFile, TExpression>`.
For type check files, we can simply print new imports as we don't expect
existing imports to be updated. That is because type check files do not
have any _original_ source files (or in practice— those are empty).
For type check blocks inline, or constructors, imports _may_ be re-used.
This is great as it helps fixing some incrementality bugs that we were
seeing in the type check code. That is, sometimes the type check block
code may generate imports conditionally for e.g. `TemplateRef`, or
animations. Those then **prevent** incremental re-use if TCB code
switches between those continously. We tried to account for that with
signal inputs by always pre-generating such imports. This fixed the
issue for type-check files, but for inline type check blocks this is
different as we would introduce new imports in user code that would then
be changed back in subsequential edit iterations. See:
https://github.com/angular/angular/pull/53521#pullrequestreview-1778130879.
In practice, the assumption was that we would be fine since user code is
most likely containing imports to `@angular/core` already. That is a
true assumption, but unfortunately it doesn't help with incremental
re-use because TypeScript's structural change detection does not dedupe
and expects 1:1 exact imports from their old source files.
https://github.com/microsoft/TypeScript/pull/56845
To improve incremental re-use for the type check integration, we should
re-use original source file imports when possible. This commit enables
this.
To update imports and execute inline operations, we are now uisng
`magic-string` (which is then bundled) as it simplifies the string
manipulatuons.
PR Close#54819
This commit switches ngtsc's JS and DTS transform to use the new import
manager. This is a drop-in replacement as we've updated the translator
helpers in the previous commit to align with the new API suggested by
the `ImportManagerV2` (to be renamed then).
PR Close#54819
`ImportGenerator` is the abstraction used by the translator functions to
insert imports for `ExternalExpr` in an AST-agnostic way.
This was built specifically for the linker which does not use any of the
complex import managers- but rather re-uses `ngImport` or uses
`ngImport.Bla`.
This commit also switches the linker AST-agnostic generator to follow
the new signatures. This was rather trivial.
PR Close#54819
This commit introduces a new implementation of `ImportManager` that has
numerous benefits:
- It allows efficient re-use of original source file imports.
* either fully re-using original imports if matching
* updating existing import declarations to include new symbols.
- It allows efficient re-use of previous generated imports.
- The manager can be used for schematics and migrations.
The implementation is a rework of the import manager that we originally
built for schematics in Angular Material, but this commit improved it
to be more flexible, more readable, and "correct".
In follow-ups we can use this for schematics/migrations.
PR Close#54819
This commit ensures components in the route config predictably always
get their providers from the hierarchy available to routes rather than
sometimes being dependent on where they are inserted.
fixes#53369
BREAKING CHANGE: Providers available to the routed components always
come from the injector heirarchy of the routes and never inherit from
the `RouterOutlet`. This means that providers available only to the
component that defines the `RouterOutlet` will no longer be available to
route components in any circumstances. This was already the case
whenever routes defined providers, either through lazy loading an
`NgModule` or through explicit `providers` on the route config.
PR Close#54265
This commit updates the logic around `Route.redirectTo` to enable using a
function to create the redirect. This function can return a string,
ands acts the same as previous string redirects, or a `UrlTree`, which
will act as an absolute redirect.
To be useful, the redirect function needs access to the params and data.
Today, developers can access these in their redirect strings, for
example `{path: ':id', redirectTo: '/user/:id'}`. Unfortunately,
developers only have access to params and data on the _current route_
today in the redirect strings. The params and data in the `RedirectFn`
give developers access to anything from the matched parent routes as
well. This is done as the same way as param and data aggregation later
on (https://github.com/angular/angular/blob/897f014785578d87bc655ea6ae9e113653960f50/packages/router/src/router_state.ts#L236-L278).
In order to accomplish this, we inherit params and data while
matching, after the `ActivatedRouteSnapshot` is created for the matched
route rather than waiting until the end.
The `RedirectFunction` does not return the full
`ActivatedRouteSnapshot` interface. Some things are not accurately known
at the route matching phase. For example, resolvers are not run until
later, so any resolved title would not be populated. The same goes for lazy
loaded components. The is also true for all the snapshots up to the
root, so properties that include parents (root, parent, pathFromRoot)
are also excluded. And naturally, the full route matching hasn't yet
happened so firstChild and children are not available either.
fixes#13373resolves#28661 (though not for the redirect string - you would return a
`UrlTree` from the function)
BREAKING CHANGE: This change allows `Route.redirectTo` to be a function
in addition to the previous string. Code which expects `redirectTo` to
only be a string on `Route` objects will need to be adjusted.
PR Close#52606
This commit updates the loop in ApplicationRef.tick to only throw in dev
mode. In addition, it reduces the reruns to 10 from 100 in order to
reduce the impact on production applications that encounter this error.
That is, the loop will bail out much earlier and prevent prolonged
unresponsiveness.
PR Close#54848
Zone event and run coalescing now races `requestAnimationFrame` and
`setTimeout`. There are tradeoffs to both functions and racing the two
gives us the benefits of both. This is explained in more detail in the
jsdoc comment. This change also aligns the timing of zone coalescing
with what will be used for zoneless.
BREAKING CHANGE: The exact timing of change detection execution when
using event or run coalescing with `NgZone` is now the first of either
`setTimeout` or `requestAnimationFrame`. Code which relies on this
timing (usually by accident) will need to be adjusted. If a callback
needs to execute after change detection, we recommend `afterNextRender`
instead of something like `setTimeout`.
fixes#54544fixes#44314fixes#39854 (unverified but seems likely)
PR Close#54578
This is a follow-up to #53718 that applies the same logic to the
`autoDetect` feature of the fixture's host view. This now unifies the
logic between `ApplicationRef` and `ComponentFixture` autodetect.
BREAKING CHANGE: The `ComponentFixture` `autoDetect` feature will no
longer refresh the component's host view when the component is `OnPush`
and not marked dirty. This exposes existing issues in components which
claim to be `OnPush` but do not correctly call `markForCheck` when they
need to be refreshed. If this change causes test failures, the easiest
fix is to change the component to `ChangeDetectionStrategy.Default`.
PR Close#54824
The logic in `isCssClassMatching` is only interested in two areas in the attributes:
implicit attributes and the `AttributeMarker.Classes` area, with the first area only
of interest for projection matching, not directive matching. This commit splits these
two searches to make this more apparent.
PR Close#54800
This commit resolves a regression that was introduced when the compiler switched from
`TemplateDefinitionBuilder` (TDB) to the template pipeline (TP) compiler. The TP compiler
has changed the output of
```html
if (false) { <div class="test"></div> }
```
from
```ts
defineComponent({
consts: [['class', 'test'], [AttributeMarker.Classes, 'test']],
template: function(rf) {
if (rf & 1) {
ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0)
}
}
});
```
to
```ts
defineComponent({
consts: [[AttributeMarker.Classes, 'test']],
template: function(rf) {
if (rf & 1) {
ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0)
}
}
});
```
The last argument to the `ɵɵtemplate` instruction (0 in both compilation outputs) corresponds with
the index in `consts` of the element's attribute's, and we observe how TP has allocated only a single
attribute array for the `div`, where there used to be two `consts` entries with TDB. Consequently,
the `ɵɵtemplate` instruction is now effectively referencing a different attributes array, where the
distinction between the `"class"` attribute vs. the `AttributeMarker.Classes` distinction affects
the behavior: TP's emit causes the runtime to incorrectly match a directive with `selector: '.foo'` to
be instantiated on the `ɵɵtemplate` instruction as if it corresponds with a structural directive!
Instead of changing TP to align with TDB's emit, this commit updates the runtime instead. This uncovered
an inconsistency in selector matching for class names, where there used to be two paths dealing with
class matching:
1. The first check was commented to be a special-case for class matching, implemented in `isCssClassMatching`.
2. The second path was part of the main selector matching algorithm, where `findAttrIndexInNode` was being used
to find the start position in `tNode.attrs` to match the selector's value against.
The second path only considers `AttributeMarker.Classes` values if matching for content projection, OR of the
`TNode` is not an inline template. The special-case in path 1 however does not make that distinction, so it
would consider the `AttributeMarker.Classes` binding as a selector match, incorrectly causing a directive to
match on the `ɵɵtemplate` itself.
The second path was also buggy for class bindings, as the return value of `classIndexOf` was incorrectly
negated: it considered a matching class attribute as non-matching and vice-versa. This bug was not observable
because of another issue, where the class-handling in part 2 was never relevant because of the special-case
in part 1.
This commit separates path 1 entirely from path 2 and removes the buggy class-matching logic in part 2, as
that is entirely handled by path 1 anyway. `isCssClassMatching` is updated to exclude class bindings from
being matched for inline templates.
Fixes#54798
PR Close#54800
Add an internal API to enable and use i18n hydration for testing and development. This helps ensure that we don't accidentally break the current behavior until we are completely ready to roll out i18n support.
PR Close#54784
Currently if an `(output)` listener fails, it will be handled gracefully
by Angular and reported to the `ErrorHandler`.
For programmatic subscriptions with `OutputEmitterRef`, this is not the case.
Instead, as soon as any subscription is failing, all other subsequent
subscription callbacks are not firing anymore.
This commit intends to make this more consistent by gracefully
reporting errors from `OutputEmitterRef#emit` to `ErrorHandler`,
allowing for listener execution to continue.
PR Close#54821
Returning `UrlTree` from a guard was a convenient new feature added to
the `Router`. However, it does not have feature-parity with the old
`router.navigate(...); return false;` pattern. The most common use-case
for this feature is to redirect to a new page _without_ updating the URL
from the initially attempted navigation. For example, rendering a 404
page when the user does not have access privelages to a route.
Fixes#17004Fixes#27148
BREAKING CHANGE: Guards can now return `RedirectCommand` for redirects
in addition to `UrlTree`. Code which expects only `boolean` or `UrlTree`
values in `Route` types will need to be adjusted.
PR Close#45023
Ensures that all of the functions intended to be run in initializers are in an injection context. This is a stop-gap until we have a compiler diagnostic for it.
PR Close#54761
This commits assert that the repeater instruction gets a reference
to a tracking function. This change will allow us to better track
occurences of https://github.com/angular/angular/issues/53628 -
in certain situations a reference to a tracking function might be
undefiened.
We are not fixing the underlying issue here, just getting better
visibility.
PR Close#54814
Speeds up the retrieval of `DestroyRef` in `EventEmitter` because
`try/catch` is expensive if there is no injection context.
We saw a script time regression in Cloud.
The goldens had to be updated because `getInjectImplementation` is now
referenced. `inject` also references the underlying field, but directly.
This is super minimal overhead of a function exposing the internal
field.
PR Close#54748
Moves the logic that creates the defer resolver function into `@angular/compiler` for consistency with the rest of the compilation APIs. Also renames some of the symbols to make it clearer what they're used for.
PR Close#54759
Fixes an issue where we were outputting the reference to non-deferrable dependencies as strings, rather than going through the reference emitter. This caused some issues internally because the reference wasn't maintained in the generated JS.
PR Close#54759
Currently we have the `deferrableDeclToImportDecl`, `deferBlocks`, `deferrableTypes` and `deferBlockDepsEmitMode` fields on the `R3ComponentMetadata` which is incorrect, because the interface is used both for JIT and AOT mode even though the information for those fields is AOT-specific. It will be problematic for partial compilation since the runtime will have a reference to the dependency loading function, but will not be able to provide any of the other information.
These changes make the following refactors:
1. It changes the defer-related information in `R3ComponentMetadata` to include only references to dependency functions which can be provided both in JIT and AOT.
2. Moves the AOT-specific defer analysis into the `ComponentResolutionData`.
3. Moves the construction the defer dependency function into the compilation phase of the `ComponentDecoratorHandler`.
4. Drops support for defer blocks from the `TemplateDefinitionBuilder`. This allows us to clean up some TDB-specific code and shouldn't have an effect on users since the TDB isn't used anymore.
PR Close#54759
This commit updates `ApplicationRef.tick` to use `detectChangesInternal` for root
views rather than go through the `ChangeDetectorRef.detectChanges` API
which refreshes the host view without first looking at whether the view
is `OnPush` and not dirty. The current behavior would hide errors in
`OnPush` components that do not correctly get marked for check and would
break when migrating to zoneless change detection because `markForCheck`
was never called so change detection was never scheduled.
The error would be surprising and blamed on switching to zoneless when in
reality the issue already exists and is a problem with the component not
calling `markForCheck`. However, this error is hidden today because
`ApplicationRef.tick` refresh host bindings unconditionally.
BREAKING CHANGE: `OnPush` views at the root of the application need to
be marked dirty for their host bindings to refresh. Previously, the host
bindings were refreshed for all root views without respecting the
`OnPush` change detection strategy.
PR Close#53718
PR Close#53718
This commit updates HTML sanitization logic to avoid infinite loops in case clobbered elements contain fields like `nextSibling` or `parentNode`. Those fields are used for DOM traversal and this update makes sure that those calls return valid results.
Also this commit fixes an issue when clobbering `nodeName` causes JS exceptions.
PR Close#54425
An i18n message effectively acts as a dynamic template: two elements with contiguous instruction indices won't necessarily be contiguous in the DOM.
For that reason, we need to maintain a mapping from instruction index to a physical DOM node in order to hydrate views with i18n, pointing to where hydration for that view should begin.
PR Close#54750
Updates the instruction generation for two-way bindings to only emit the `twoWayBindingSet` call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking.
Fixes#54670.
PR Close#54714
We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by #54154 because two-way bindings no longer had a `PropertyWrite` AST.
These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals.
PR Close#54714
Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes.
PR Close#54714
Introduces a new `TemplateSemanticsChecker` that will be used to flag semantic errors in the user's template. Currently we do some of this in the type check block, but the problem is that it doesn't have access to the template type checker which prevents us from properly checking cases like #54670. This pass is also distinct from the extended template checks, because we don't want users to be able to turn the checks off and we want them to run even if `strictTemplates` are disabled.
PR Close#54714
The `runCallbackOnce` closure is declared not to have any parameters itself, so it is
compatible as `queueMicrotask` callback without the extra closure. This reduces the call
stack by a frame and avoids the extra closure allocation.
PR Close#54801
This commit addresses a typing mismatch, where these functions were declared to return whichever
value their callback returned, but this was inaccurate: it's always a test callback function
with `done` argument.
PR Close#54801
The assertion in `packages/core/test/acceptance/after_render_hook_spec.ts:165` was prone to flakes,
where Jasmine could frequently report an error:
```
Error: 'expect' was used when there was no current spec, this could be because an asynchronous test timed out
at Env.expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1945:15)
at expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:8267:18)
at file:///packages/core/test/acceptance/after_render_hook_spec.ts:165:12
```
This happens because `wrapTestFn` checks for an exact type of `Promise`, which may have been patched by zone.js
such that the `instanceof` condition is dependent on whether zone.js has patched the `Promise` constructor.
PR Close#54801
Many acceptance tests define goldens for both `TemplateDefinitionBuilder` and Template Pipeline. All such tests have had the TDB golden files removed, and the corresponding expectations adjusted.
PR Close#54757
`TemplateDefinitionBuilder` is the legacy template compiler, and was replaced by Template Pipeline as the default in v17.3.
This PR attempts to delete `TemplateDefinitionBuilder`, `ExpressionConverter`, and various helpers (i18n context, style builder, property visitors, etc).
Consider this a first pass: a lot of code has not yet been deleted (e.g. old TDB-specific test cases), and I'm sure I have missed additional helper code.
PR Close#54757
There is an edge case where synchronous navigations caused in
response to navigation events can result in a previous navigation not
being unsubscribed from. b/328219996
PR Close#54710
This commit ensures that render hooks are rerun when a node is attached
or detached. We do not necessarily need to run change detection but DOM
did change so render hooks should execute.
PR Close#54083
This commit updates the `afterRender` and `afterNextRender` hooks to
notify the scheduler (which subsequently schedules change detection)
when created. This makes the hooks similar to `requestAnimationFrame`,
which requests that the browser schedule a rendering operation. This
reqeust is not conditional. Even if there was nothing to repaint, the
`requestAnimationFrame` callback will execute.
In Angular, this is useful because callers of `afterNextRender` don't
necessarily have any way of knowing whether a change detection is even
scheduled. For example, the anchor scrolling with the Angular Router
needs to wait for rendering to complete before attempting to scroll
because rendering can affect the size of the page. However, if the user
is already on the page that the navigation is targeting, such as
navigating to an anchor on the page, there is nothing new for the Router
to render so a render might not even be scheduled.
Related to https://github.com/angular/angular/issues/53985, which
could use `afterNextRender` instead of `setTimeout` to ensure the
scrolling happens in the same frame as the page rendering, but would not
necessarily work without this change (as described above). Note that the
scrolling _cannot_ use a microtask to ensure scrolling happens in the
same frame because `NgZone` will ensure microtasks flush before
change detection, so it would cause the scroll to happen before rendering.
PR Close#54083
Paul Gschwendtner