This commit fixes a regression from "fix(common): ensure
scrollRestoration is writable (#30630)" that caused scrolling to not
happen at all in browsers that do not support scroll restoration. The
issue was that `supportScrollRestoration` was updated to return `false`
if a browser did not have a writable `scrollRestoration`. However, the
previous behavior was that the function would return `true` if
`window.scrollTo` was defined. Every scrolling function in the
`ViewportScroller` used `supportScrollRestoration` and, with the update
in bb88c9fa3d, no scrolling would be
performed if a browser did not have writable `scrollRestoration` but
_did_ have `window.scrollTo`.
Note, that this failure was detected in the saucelabs tests. IE does not
support scroll restoration so IE tests were failing.
PR Close#38468
When removal of one view causes removal of another one from the same
ViewContainerRef it triggers an error with views length calculation. This commit
fixes this bug by removing a view from the list of available views before invoking
actual view removal (which might be recursive and relies on the length of the list
of available views).
Fixes#38201.
PR Close#38317
For a template that contains for example `<span *ngIf="first"></span>`
there's no need to render the `NgIf` guard expression, as the child
scope does not have any type-checking statements, so any narrowing
effect of the guard is not applicable.
This seems like a minor improvement, however it reduces the number of
flow-node antecedents that TypeScript needs to keep into account for
such cases, resulting in an overall reduction of type-checking time.
PR Close#38418
The template type-checker would always generate a directive declaration
even if its type was never used. For example, directives without any
input nor output bindings nor exportAs references don't need the
directive to be declared, as its type would never be used.
This commit makes the `TcbOp`s that are responsible for declaring a
directive as optional, such that they are only executed when requested
from another operation.
PR Close#38418
The template type-checker would generate a statement with a call
expression for all DOM elements in a template of the form:
```
const _t1 = document.createElement("div");
```
Profiling has shown that this is a particularly expensive call to
perform type inference on, as TypeScript needs to perform signature
selection of `Document.createElement` and resolve the exact type from
the `HTMLElementTagNameMap`. However, it can be observed that the
statement by itself does not contribute anything to the type-checking
result if `_t1` is not actually used anywhere, which is only rarely the
case---it requires that the element is referenced by its name from
somewhere else in the template. Consequently, the type-checker can skip
generating this statement altogether for most DOM elements.
The effect of this optimization is significant in several phases:
1. Less type-check code to generate
2. Less type-check code to emit and parse again
3. No expensive type inference to perform for the call expression
The effect on phase 3 is the most significant here, as type-checking is
not currently incremental in the sense that only phases 1 and 2 can
be reused from a prior compilation. The actual type-checking of all
templates in phase 3 needs to be repeated on each incremental
compilation, so any performance gains we achieve here are very
beneficial.
PR Close#38418
This commit replaces the old and slow `ReflectiveInjector` that was
deprecated in v5 with the new `Injector`. Note: This change was only
done in the spec files inside the `aio` folder.
While changing this, it was not possible to directly use `Injector.get`
to get the correct typing for the mocked classes. For example:
```typescript
locationService = injector.get<TestLocationService>(LocationService);
```
Fails with:
> Argument of type 'typeof LocationService' is not assignable to parameter
of type 'Type<TestLocationService> | InjectionToken<TestLocationService> |
AbstractType<TestLocationService>'.
Type 'typeof LocationService' is not assignable to type 'Type<TestLocationService>'.
Property 'searchResult' is missing in type 'LocationService' but required in type
'TestLocationService'.
Therefore, it was necessary to first convert to `unknown` and then to
`TestLocationService`.
```typescript
locationService = injector.get(LocationService) as unknown as TestLocationService;
```
PR Close#38408
We should define ngDevMode to false in Closure, but --define only works in the global scope.
With ngDevMode not being set to false, this size tracking test provides little value but a lot of
headache to continue updating the size.
PR Close#38449
The `@HostListener` functions and lifecycle hooks aren't intended to be public API but
do need to appear in the `.d.ts` files or type checking will break. Adding the
nodoc annotation will correctly hide this function on the docs site.
Again, note that `@internal` cannot be used because the result would be
that the functions then do not appear in the `.d.ts` files. This would
break lifecycle hooks because the class would be seen as not
implementing the interface correctly. This would also break
`HostListener` because the compiled templates would attempt to call the
`onClick` functions, but those would also not appear in the `d.ts` and
would produce errors like "Property 'onClick' does not exist on type 'RouterLinkWithHref'".
PR Close#38448
Fixes an error if a CSS custom property, used inside a host binding, has a
number in its name. The error is thrown because the styling parser only
expects characters from A to Z,dashes, underscores and a handful of other
characters.
Fixes#37292.
PR Close#38432
Previously we added a browser target for `firefox` into the
dev-infra package. It looks like as part of this change, we
accidentally switched the local web testing target to `firefox`.
Web tests are not commonly run locally as we use Domino and
NodeJS tests for primary development. Sometimes though we intend
to run tests in a browser. This would currently work with Firefox
but not on Windows (as Firefox is a noop there in Bazel).
This commit switches the primary browser back to `chromium`. Also
Firefox has been added as a second browser to web testing targets.
This allows us to reduce browsers in the legacy Saucelabs job. i.e.
not running Chrome and Firefox there. This should increase stability
and speed up the legacy job (+ reduced rate limit for Saucelabs).
PR Close#38435
Previously commit message types were provided as part of the ng-dev config in the repository
using the ng-dev toolset. This change removes this configuration expectation and instead
predefines the valid types for commit messages.
Additionally, with this new unified set of types requirements around providing a scope have
been put in place. Scopes are either required, optional or forbidden for a given commit
type.
PR Close#38430
We recently updated the benchpress package to have a more loose
Angular core peer dependency, and less other unused dependencies.
We should make sure to use that in the dev-infra package so that
peer dependencies can be satisified in consumer projects, and so
that less unused dependencies are brought into projects.
PR Close#38440
When a commit message fails validation, rather than throwing out the commit message entirely
the commit message is saved into a draft file and restored on the next commit attempt.
PR Close#38304
Runs the `ng-dev format changed` command whenever the `git commit` command is
run. As all changes which are checked by CI will require this check passing, this
change can prevent needless roundtrips to correct lint/formatting errors. This
automatic formatting can be bypassed with the `--no-verify` flag on the `git commit`
command.
PR Close#38402
Adds an ng-dev formatter option to format all of the staged files. This will can
be used to format only the staged files during the pre-commit hook.
PR Close#38402
Currently the Closure-related tests are not tree-shaking the dev-mode-only content, thus payload
size checks are failing even if dev-mode-only content is added.
The 2e9fdbde9e commit
added some logic to JIT compiler, which is likely triggered the payload size increase. This commit
updates the payload size limits for Closure-related test to get master and patch branches back to
the "green" state.
PR Close#38411
The compiler does not currently report errors when there's an `@Input()`
for a `private`, `protected`, or `readonly` directive/component class member.
This change adds an option to enable reporting errors when a template
attempts to bind to one of these restricted input fields.
PR Close#38249
Prior to this change, the template type checker would always use a
type-constructor to instantiate a directive. This type-constructor call
serves two purposes:
1. Infer any generic types for the directive instance from the inputs
that are passed in.
2. Type check the inputs that are passed into the directive's inputs.
The first purpose is only relevant when the directive actually has any
generic types and using a type-constructor for these cases inhibits
a type-check performance penalty, as a type-constructor's signature is
quite complex and needs to be generated for each directive.
This commit refactors the generated type-check blocks to only generate
a type-constructor call for directives that have generic types. Type
checking of inputs is achieved by generating individual statements for
all inputs, using assignments into the directive's fields.
Even if a type-constructor is used for type-inference of generic types
will the input checking also be achieved using the individual assignment
statements. This is done to support the rework of the language service,
which will start to extract symbol information from the type-check
blocks.
As a future optimization, it may be possible to reduce the number of
inputs passed into a type-constructor to only those inputs that
contribute the the type-inference of the generics. As this is not a
necessity at the moment this is left as follow-up work.
Closes#38185
PR Close#38249
"Quote expressions" are expressions that start with an identifier followed by a
comma, allowing arbitrary syntax to follow. These kinds of expressions would
throw a an error in the template type checker, which would make them hard to
track down. As quote expressions are not generally used at all, the error would
typically occur for URLs that would inadvertently occur in a binding:
```html
<a [href]="https://example.com"></a>
```
This commit lets such bindings be inferred as the `any` type.
Fixes#36568
Resolves FW-2051
PR Close#37917
In TypeScript 3.8 support was added for type-only imports, which only brings in
the symbol as a type, not their value. The Angular compiler did not yet take
the type-only keyword into account when representing symbols in type positions
as value expressions. The class metadata that the compiler emits would include
the value expression for its parameter types, generating actual imports as
necessary. For type-only imports this should not be done, as it introduces an
actual import of the module that was originally just a type-only import.
This commit lets the compiler deal with type-only imports specially, preventing
a value expression from being created.
Fixes#37900
PR Close#37912
When using the safe navigation operator in a binding expression, a temporary
variable may be used for storing the result of a side-effectful call.
For example, the following template uses a pipe and a safe property access:
```html
<app-person-view [enabled]="enabled" [firstName]="(person$ | async)?.name"></app-person-view>
```
The result of the pipe evaluation is stored in a temporary to be able to check
whether it is present. The temporary variable needs to be declared in a separate
statement and this would also cause the full expression itself to be pulled out
into a separate statement. This would compile into the following
pseudo-code instructions:
```js
var temp = null;
var firstName = (temp = pipe('async', ctx.person$)) == null ? null : temp.name;
property('enabled', ctx.enabled)('firstName', firstName);
```
Notice that the pipe evaluation happens before evaluating the `enabled` binding,
such that the runtime's internal binding index would correspond with `enabled`,
not `firstName`. This introduces a problem when the pipe uses `WrappedValue` to
force a change to be detected, as the runtime would then mark the binding slot
corresponding with `enabled` as dirty, instead of `firstName`. This results
in the `enabled` binding to be updated, triggering setters and affecting how
`OnChanges` is called.
In the pseudo-code above, the intermediate `firstName` variable is not strictly
necessary---it only improved readability a bit---and emitting it inline with
the binding itself avoids the out-of-order execution of the pipe:
```js
var temp = null;
property('enabled', ctx.enabled)
('firstName', (temp = pipe('async', ctx.person$)) == null ? null : temp.name);
```
This commit introduces a new `BindingForm` that results in the above code to be
generated and adds compiler and acceptance tests to verify the proper behavior.
Fixes#37194
PR Close#37911
In JIT compiled apps, component definitions are compiled upon first
access. For a component class `A` that extends component class `B`, the
`B` component is also compiled when the `InheritDefinitionFeature` runs
during the compilation of `A` before it has finalized. A problem arises
when the compilation of `B` would flush the NgModule scoping queue,
where the NgModule declaring `A` is still pending. The scope information
would be applied to the definition of `A`, but its compilation is still
in progress so requesting the component definition would compile `A`
again from scratch. This "inner compilation" is correctly assigned the
NgModule scope, but once the "outer compilation" of `A` finishes it
would overwrite the inner compilation's definition, losing the NgModule
scope information.
In summary, flushing the NgModule scope queue could trigger a reentrant
compilation, where JIT compilation is non-reentrant. To avoid the
reentrant compilation, a compilation depth counter is introduced to
avoid flushing the NgModule scope during nested compilations.
Fixes#37105
PR Close#37795
When navigations coming from Angular router we may have a payload stored in state property. When this
exists, set extras's state to the payload.
PR Close#28176
The changes in https://github.com/angular/angular/pull/38368 split `render3/i18n.ts` files into
smaller scripts, but the PullApprove config was not updated to reflect that. This commit updates
the PullApprove config to reflect the recent changes in i18n-related files.
PR Close#38403
Queries weren't matching directives that provide themselves via string
injection tokens, because the assumption was that any string passed to
a query decorator refers to a template reference.
These changes make it so we match both template references and
providers while giving precedence to the template references.
Fixes#38313.
Fixes#38315.
PR Close#38321
When we were outputting class members for `setClassMetadata` calls,
we were using the string representation of the member name. This can
lead to us generating invalid code when the name contains dashes and
is quoted (e.g. `@Output() 'has-dashes' = new EventEmitter()`), because
the quotes will be stripped for the string representation.
These changes fix the issue by using the original name AST node that was
used for the declaration and which knows whether it's supposed to be
quoted or not.
Fixes#38311.
PR Close#38387
This commit contains no changes to code. It only breaks `i18n.ts` file
into `i18n.ts` + `i18n_apply.ts` + `i18n_parse.ts` +
`i18n_postprocess.ts` for easier maintenance.
PR Close#38368
Defer loading the wildcard module so that it is not loaded until
subscribed to. This fixes an issue where it was being eagerly loaded.
As an example, wildcard module loading should only occur after all other potential
matches have been exhausted. A test case for this was also added to
demonstrate the fix.
Fixes#25494
PR Close#38348
When a ServerStylesHost instance is destroyed, all of the shared styles added to the DOM
head element by that instance should be removed. Without this removal, over time a large
number of style rules will build up and cause extra memory pressure. This brings the
ServerStylesHost in line with the DomStylesHost used by the platform browser, which
performs this same cleanup.
PR Close#38367
The currently selected ICU was incorrectly being stored it `TNode`
rather than in `LView`.
Remove: `TIcuContainerNode.activeCaseIndex`
Add: `LView[TIcu.currentCaseIndex]`
PR Close#38345
This commit performs minor refactoring in Forms package to get rid of duplicate functions.
It looks like the functions were duplicated due to a slightly different type signatures, but
their logic is completely identical. The logic in retained functions remains the same and now
these function also accept a generic type to achieve the same level of type safety.
PR Close#38371
This commit uses getElementById and getElementsByName when an anchor scroll happens,
to avoid escaping the anchor and wrapping the code in a try/catch block.
Related to #28960
PR Close#30143
This change provides better typing for the `LView.debug` property which
is intended to be used by humans while debugging the application with
`ngDevMode` turned on.
In addition this chang also adds jasmine matchers for better asserting
that `LView` is in the correct state.
PR Close#38359
The Chrome debugger is not able to render the syntax properly when the
code contains backticks. This is a known issue in Chrome and they have an
open [issue](https://bugs.chromium.org/p/chromium/issues/detail?id=659515) for that.
This commit adds the work-around to use double backslash with one
backtick ``\\` `` at the end of the line.
This can be reproduced by running the following command:
`yarn bazel test //packages/forms/test --config=debug`
When opening the chrome debugger tools, you should see the correct
code highlighting syntax.
PR Close#38332
For attribute bindings that target a directive's input, the template
type checker is able to verify that the type of the input expression is
compatible with the directive's declaration for said input. This
checking adheres to the `strictNullChecks` flag as configured in the
TypeScript compilation, such that errors are reported for expressions
that include `undefined` or `null` in their type if the input's
declaration does not include those types.
There was a bug with this level of type-checking for directives that
also declare coercion members, where binding an expression that includes
the `undefined` type to a directive's input that does not include the
`undefined` type would not be reported as error.
This commit fixes the bug by changing the type-constructor in type-check
code to use an intersection type of regular inputs and coerced inputs,
instead of a union type. The union type would inadvertently allow
`undefined` types to be assigned into the regular inputs, as that would
still satisfy the characteristics of a union type.
As a result of this change, you may start to see build failures if
`strictTemplates` is enabled and `strictInputTypes` is not disabled.
These errors are legitimate and some action is required to achieve a
successful build:
1. Update the templates for which an error is reported and introduce the
non-null assertion operator at the end of the expression. This
removes the `undefined` type from the expression's type, making it
appear as a valid assignment.
2. Disable `strictNullInputTypes` in the compiler options. This will
implicitly add the non-null assertion operators similar to option 1,
but all templates in the compilation are affected.
3. Update the directive's input declaration to include the `undefined`
type, if the directive is not implemented in an external library.
PR Close#38273
Andrew Scott
Anas Barghoud
waterplea
Ahn
JoostK
Sonu Kapoor
crisbeto
Paul Gschwendtner
Joey Perrott
Sergey Falinsky
Alan Agius
Andrew Kushnir
Vlad GURDIGA
Alexander Vologin
Misko Hevery
Zach Pomerantz
Gillan Martindale
cindygk
Shmuela Jacobs
Alessandro