Commit Graph

811 Commits

Author SHA1 Message Date
Kristiyan Kostadinov a61fe96b70 fix(compiler-cli): incorrectly type checking calls to implicit template variables (#39686)
Currently when we encounter an implicit method call (e.g. `{{ foo(1) }}`) and we manage to resolve
its receiver to something within the template, we assume that the method is on the receiver itself
so we generate a type checking code to reflect it. This assumption is true in most cases, but it
breaks down if the call is on an implicit receiver and the receiver itself is being invoked. E.g.

```
<div *ngFor="let fn of functions">{{ fn(1) }}</div>
```

These changes resolve the issue by generating a regular function call if the method call's receiver
is pointing to `$implicit`.

Fixes #39634.

PR Close #39686
2020-11-16 09:36:10 -08:00
Andrew Scott 7e724add7e refactor(compiler-cli): Add additional key/value spans to TCB (#39665)
In order to more accurately map from a node in the TCB to a template position,
we need to provide more span information in the TCB. These changes are necessary
for the Language Service to map from a TCB node back to a specific
locations in the template for actions like "find references" and
"refactor/rename". After the TS "find references" returns results,
including those in the TCB, we need to map specifically to the matching
key/value spans in the template rather than the entire source span.

This also has the benefit of producing diagnostics which align more
closely with what TypeScript produces.
The following example shows TS code and the diagnostic produced by an invalid assignment to a property:

```
let a: {age: number} = {} as any;
a.age = 'laksjdf';
^^^^^ <-- Type 'string' is not assignable to type 'number'.
```
A corollary to this in a template file would be [age]="'someString'". The diagnostic we currently produce for this is:

```
Type 'number' is not assignable to type 'string'.

1 <app-hello [greeting]="1"></app-hello>
             ~~~~~~~~~~~~~~
```
Notice that the underlined text includes the entire span.
If we included the keySpan for the assignment to the property,
this diagnostic underline would be more similar to the one produced by TypeScript;
that is, it would only underline “greeting”.

[design/discussion doc]
(https://docs.google.com/document/d/1FtaHdVL805wKe4E6FxVTnVHl38lICoHIjS2nThtRJ6I/edit?usp=sharing)

PR Close #39665
2020-11-16 09:33:11 -08:00
Alex Rickabaugh c59f401f9a fix(compiler-cli): setComponentScope should only list used components/pipes (#39662)
ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition `directiveDefs` and `pipeDefs`. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to `setComponentScope` in the component's NgModule file is
used to patch `directiveDefs` and `pipeDefs` onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

1. it breaks under `sideEffects: false` logic in bundlers including the CLI
2. it breaks tree-shaking for the given component and its dependencies

See this doc for further details: https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful `setComponentScope` call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of `directiveDefs` in
the component definition itself, `setComponentScope` calls had to list the
_entire_ compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. `CommonModule`), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in `setComponentScope`
calls, which should ameliorate the tree-shaking impact somewhat.

PR Close #39662
2020-11-13 11:57:20 -08:00
Pete Bacon Darwin eea5d6b38f test(compiler-cli): convert element related compliance tests (#39617)
This commit converts a set of compliance tests in the `r3_compiler_compliance_spec.ts`
file to the new testing approach.

PR Close #39617
2020-11-13 11:25:57 -08:00
Pete Bacon Darwin e719419d3f test(compiler-cli): create initial "linked compile" compliance tests (#39617)
This commit adds bazel rules to test whether linking the golden partial
files for test cases produces the same output as a full compile of the
test case would.

PR Close #39617
2020-11-13 11:25:57 -08:00
Pete Bacon Darwin 10525af67b test(compiler-cli): generate golden files for partial compilation (#39617)
This commit adds a JS script that can generate a partial golden file
for test cases in the compiler compliance tests.

PR Close #39617
2020-11-13 11:25:56 -08:00
Pete Bacon Darwin 793d66afa5 test(compiler-cli): create "full compile" compliance test rules (#39617)
This commit contains the basic runner logic and a couple of sample test cases
for the "full compile" compliance tests, where source files are compiled
to full definitions and checked against expectations.

PR Close #39617
2020-11-13 11:25:56 -08:00
Pete Bacon Darwin 8d445e0dff refactor(compiler-cli): move legacy compliance tests to new folder (#39617)
This commit renames the original `compliance` test directory to `compliance_old`.
Eventually this directory will be deleted once all the tests have been
migrated to the new test case based compliance tests.

PR Close #39617
2020-11-13 11:25:56 -08:00
Pete Bacon Darwin b9b9178458 fix(compiler-cli): do not drop non-Angular decorators when downleveling (#39577)
There is a compiler transform that downlevels Angular class decorators
to static properties so that metadata is available for JIT compilation.
The transform was supposed to ignore non-Angular decorators but it was
actually completely dropping decorators that did not conform to a very
specific syntactic shape (i.e. the decorator was a simple identifier, or
a namespaced identifier).

This commit ensures that all non-Angular decorators are kepts as-is
even if they are built using a syntax that the Angular compiler does not
understand.

Fixes #39574

PR Close #39577
2020-11-06 09:21:51 -08:00
Kristiyan Kostadinov 27ae0604c6 test(compiler-cli): resolve test failure (#39568)
Narrows down an assertion in one of our tests so it's less prone to failure due to
extra commas.

PR Close #39568
2020-11-04 12:02:09 -08:00
JoostK ded7beed32 test(compiler-cli): run the linker transform in the compliance test's prelink target (#39518)
In PR #38938 an additional Bazel target was introduced for the compliance
tests, as preparation to run the compliance tests in partial compilation
mode and then apply the linker transform. The linker plugin itself was
not available at the time but has since been implemented, so this commit
updates the prelink target of the compliance tests to apply the linker
transform using the Babel plugin.

Actually emitting partial compilations to be transformed will be done in
follow-up work.

PR Close #39518
2020-11-04 10:44:37 -08:00
Kristiyan Kostadinov 7e33cb9626 fix(compiler-cli): generating invalid setClassMetadata call in ES5 for class with custom decorator (#39527)
When a class with a custom decorator is transpiled to ES5, it looks something like this:

```
var SomeClass = (function() {
  function SomeClass() {...};
  var SomeClass_1 = __decorate([Decorator()], SomeClass);
  SomeClass = SomeClass_1;
  return SomeClass;
})();
```

The problem is that if the class also has an Angular decorator that refers to the class itself
(e.g. `{provide: someToken, useClass: SomeClass}`), the generated `setClassMetadata` code will
be emitted after the IIFE, but will still refer to the intermediate `SomeClass_1` variable from
inside the IIFE. This happens, because we generate the `setClassMetadata` call directly from
the source AST which contains identifiers that TS will rename when it emits the ES5 code.

These changes resolve the issue by looking through the metadata AST and cloning any `Identifier`
that is referring to the class. Since TS doesn't have references to the clone, it won't rename
it when transpiling to ES5.

Fixes #39509.

PR Close #39527
2020-11-03 14:52:59 -08:00
JoostK 3b0b7d2210 fix(compiler-cli): report missing pipes when `fullTemplateTypeCheck` is disabled (#39320)
Even if `fullTemplateTypeCheck` is disabled should missing pipes still
be reported, as was the case in View Engine.

Fixes #38195

PR Close #39320
2020-10-30 18:01:51 -07:00
Kristiyan Kostadinov cbc0907bfd fix(compiler): preserve this.$event and this.$any accesses in expressions (#39323)
Currently expressions `$event.foo()` and `this.$event.foo()`, as well as `$any(foo)` and
`this.$any(foo)`, are treated as the same expression by the compiler, because `this` is considered
the same implicit receiver as when the receiver is omitted. This introduces the following issues:

1. Any time something called `$any` is used, it'll be stripped away, leaving only the first parameter.
2. If something called `$event` is used anywhere in a template, it'll be preserved as `$event`,
rather than being rewritten to `ctx.$event`, causing the value to undefined at runtime. This
applies to listener, property and text bindings.

These changes resolve the first issue and part of the second one by preserving anything that
is accessed through `this`, even if it's one of the "special" ones like `$any` or `$event`.
Furthermore, these changes only expose the `$event` global variable inside event listeners,
whereas previously it was available everywhere.

Fixes #30278.

PR Close #39323
2020-10-30 10:49:15 -07:00
Alan Agius dd0ba3f4ab docs: rename `ng xi18n` to `ng extract-i18n` (#39337)
In Angular CLI version 11, xi18n has been changed from `ng xi18n` to `ng extract-i18n`.

PR Close #39337
2020-10-28 14:42:27 -07:00
Kristiyan Kostadinov 58408d6a60 fix(compiler): treat i18n attributes with no bindings as static attributes (#39408)
Currently `i18n` attributes are treated the same no matter if they have data bindings or not. This
both generates more code since they have to go through the `ɵɵi18nAttributes` instruction and
prevents the translated attributes from being injected using the `@Attribute` decorator.

These changes makes it so that static translated attributes are treated in the same way as regular
static attributes and all other `i18n` attributes go through the old code path.

Fixes #38231.

PR Close #39408
2020-10-27 13:31:29 -07:00
Pete Bacon Darwin 2736a43ecb fix(compiler-cli): support namespaced query types in directives (#38959)
Previously directive "queries" that relied upon a namespaced type

```ts
queries: {
  'mcontent': new core.ContentChild('test2'),
}
```

caused an error to be thrown. This is now supported.

PR Close #38959
2020-10-12 08:32:47 -07:00
Pete Bacon Darwin 0accd1e68d refactor(compiler-cli): implement `DeclarationNode` node type (#38959)
Previously the `ConcreteDeclaration` and `InlineDeclaration` had
different properties for the underlying node type. And the `InlineDeclaration`
did not store a value that represented its declaration.

It turns out that a natural declaration node for an inline type is the
expression. For example in UMD/CommonJS this would be the `exports.<name>`
property access node.

So this expression is now used for the `node` of `InlineDeclaration` types
and the `expression` property is dropped.

To support this the codebase has been refactored to use a new `DeclarationNode`
type which is a union of `ts.Declaration|ts.Expression` instead of `ts.Declaration`
throughout.

PR Close #38959
2020-10-12 08:32:46 -07:00
Kristiyan Kostadinov 4a1c12c773 feat(core): remove ViewEncapsulation.Native (#38882)
Removes `ViewEncapsulation.Native` which has been deprecated for several major versions.

BREAKING CHANGES:
* `ViewEncapsulation.Native` has been removed. Use `ViewEncapsulation.ShadowDom` instead. Existing
usages will be updated automatically by `ng update`.

PR Close #38882
2020-10-08 11:56:03 -07:00
Pete Bacon Darwin 5903e8ad65 test(compiler-cli): run compliance tests for two compilation modes … (#38938)
To verify the correctness of the linker output, we leverage the existing
compliance tests. The plan is to test the linker by running all compliance
tests using a full round trip of pre-linking and subsequently post-linking,
where the generated code should be identical to a full AOT compile.

This commit adds an additional Bazel target that runs the compliance
tests in partial mode. Follow-up work is required to implement the logic
for running the linker round trip.

PR Close #38938
2020-09-30 12:49:16 -07:00
JoostK 06525cfed3 test(compiler-cli): fix tests to have at least one component (#39011)
With the introduction of incremental type checking in #36211, an
intermediate `ts.Program` for type checking is only created if there are
any templates to check. This rendered some tests ineffective at avoiding
regressions, as the intermediate `ts.Program` was required for the tests
to fail if the scenario under test would not be accounted for. This
commit adds a single component to these tests, to ensure the
intermediate `ts.Program` is in fact created.

PR Close #39011
2020-09-28 16:27:34 -04:00
JoostK e9a8f9f705 fix(compiler-cli): enable @types discovery in incremental rebuilds (#39011)
Prior to this fix, incremental rebuilds could fail to type check due to
missing ambient types from auto-discovered declaration files in @types
directories, or type roots in general. This was caused by the
intermediary `ts.Program` that is created for template type checking,
for which a `ts.CompilerHost` was used which did not implement the
optional `directoryExists` methods. As a result, auto-discovery of types
would not be working correctly, and this would retain into the
`ts.Program` that would be created for an incremental rebuild.

This commit fixes the issue by forcing the custom `ts.CompilerHost` used
for type checking to properly delegate into the original
`ts.CompilerHost`, even for optional methods. This is accomplished using
a base class `DelegatingCompilerHost` which is typed in such a way that
newly introduced `ts.CompilerHost` methods must be accounted for.

Fixes #38979

PR Close #39011
2020-09-28 16:27:34 -04:00
Kristiyan Kostadinov 3f9be429fc test(compiler-cli): error when running tests on non-posix systems (#39005)
We weren't resolving a path correctly which resulted in an error on Windows.
For reference, here's the error. Note the extra slash before `C:`:

```
Error: ENOENT: no such file or directory, scandir '/C:/bazel_output_root/yxvwd24o/external/npm/node_modules/typescript'
    at Object.readdirSync (fs.js:854:3)
```

PR Close #39005
2020-09-28 16:27:01 -04:00
JoostK e790c8547e test(compiler-cli): load test files into memory only once (#38909)
Prior to this change, each invocation of `loadStandardTestFiles` would
load the necessary files from disk. This function is typically called
at the top-level of a test module in order to share the result across
tests. The `//packages/compiler-cli/test/ngtsc` target has 8 modules
where this call occurs, each loading their own copy of
`node_modules/typescript` which is ~60MB in size, so the memory overhead
used to be significant. This commit loads the individual packages into
a standalone `Folder` and mounts this folder into the filesystem of
standard test files, such that all file contents are no longer
duplicated in memory.

PR Close #38909
2020-09-25 14:28:49 -04:00
JoostK b627f7f02e test(compiler-cli): improve test performance using shared source file cache (#38909)
Some compiler tests take a long time to run, even using multiple
executors. A profiling session revealed that most time is spent in
parsing source files, especially the default libraries are expensive to
parse.

The default library files are constant across all tests, so this commit
introduces a shared cache of parsed source files of the default
libraries. This achieves a significant improvement for several targets
on my machine:

//packages/compiler-cli/test/compliance: from 23s to 5s.
//packages/compiler-cli/test/ngtsc: from 115s to 11s.

Note that the number of shards for the compliance tests has been halved,
as the extra shards no longer provide any speedup.

PR Close #38909
2020-09-25 14:28:49 -04:00
Andrew Scott bd7d8744fa test(core): enable test in compiler compliance for namespace uri (#38957)
Enables test that was fixed by #24386.
resolves #24426.

PR Close #38957
2020-09-24 11:35:43 -04:00
Alex Rickabaugh 40975e06c6 fix(compiler-cli): perform DOM schema checks even in basic mode in g3 (#38943)
In Ivy, template type-checking has 3 modes: basic, full, and strict. The
primary difference between basic and full modes is that basic mode only
checks the top-level template, whereas full mode descends into nested
templates (embedded views like ngIfs and ngFors). Ivy applies this approach
to all of its template type-checking, including the DOM schema checks which
validate whether an element is a valid component/directive or not.

View Engine has both the basic and the full mode, with the same distinction.
However in View Engine, DOM schema checks happen for the full template even
in the basic mode.

Ivy's behavior here is technically a "fix" as it does not make sense for
some checks to apply to the full template and others only to the top-level
view. However, since g3 relies exclusively on the basic mode of checking and
developers there are used to DOM checks applying throughout their template,
this commit re-enables the nested schema checks even in basic mode only in
g3. This is done by enabling the checks only when Closure Compiler
annotations are requested.

Outside of g3, it's recommended that applications use at least the full mode
of checking (controlled by the `fullTemplateTypeCheck` flag), and ideally
the strict mode (`strictTemplates`).

PR Close #38943
2020-09-23 15:46:32 -04:00
Pete Bacon Darwin 123bff7cb6 fix(compiler-cli): generate `let` statements in ES2015+ mode (#38775)
When the target of the compiler is ES2015 or newer then we should
be generating `let` and `const` variable declarations rather than `var`.

PR Close #38775
2020-09-21 12:27:27 -07:00
Pete Bacon Darwin d795a00137 refactor(compiler): replace Comment nodes with leadingComments property (#38811)
Common AST formats such as TS and Babel do not use a separate
node for comments, but instead attach comments to other AST nodes.
Previously this was worked around in TS by creating a `NotEmittedStatement`
AST node to attach the comment to. But Babel does not have this facility,
so it will not be a viable approach for the linker.

This commit refactors the output AST, to remove the `CommentStmt` and
`JSDocCommentStmt` nodes. Instead statements have a collection of
`leadingComments` that are rendered/attached to the final AST nodes
when being translated or printed.

PR Close #38811
2020-09-18 08:01:25 -07:00
JoostK a32a317ea1 fix(compiler-cli): ensure that a declaration is available in type-to-value conversion (#38684)
The type-to-value conversion could previously crash if a symbol was
resolved that does not have any declarations, e.g. because it's imported
from a missing module. This would typically result in a semantic
TypeScript diagnostic and halt further compilation, therefore not
reaching the type-to-value conversion logic. In Bazel however, it turns
out that Angular semantic diagnostics are requested even if there are
semantic TypeScript errors in the program, so it would then reach the
type-to-value conversation and crash.

This commit fixes the unsafe access and adds a test that ignores the
TypeScript semantic error, effectively replicating the situation as
experienced under Bazel.

Fixes #38670

PR Close #38684
2020-09-08 14:06:25 -07:00
Pete Bacon Darwin 7e0b3fd953 fix(compiler-cli): compute source-mappings for localized strings (#38645)
Previously, localized strings had very limited or incorrect source-mapping
information available.

Now the i18n AST nodes and related output AST nodes include source-span
information about message-parts and placeholders - including closing tag
placeholders.

This information is then used when generating the final localized string
ASTs to ensure that the correct source-mapping is rendered.

See #38588 (comment)

PR Close #38645
2020-09-08 13:17:21 -07:00
Alex Rickabaugh c90eb5450d refactor(compiler-cli): make template parsing errors into diagnostics (#38576)
Previously, the compiler was not able to display template parsing errors as
true `ts.Diagnostic`s that point inside the template. Instead, it would
throw an actual `Error`, and "crash" with a stack trace containing the
template errors.

Not only is this a poor user experience, but it causes the Language Service
to also crash as the user is editing a template (in actuality the LS has to
work around this bug).

With this commit, such parsing errors are converted to true template
diagnostics with appropriate span information to be displayed contextually
along with all other diagnostics. This majorly improves the user experience
and unblocks the Language Service from having to deal with the compiler
"crashing" to report errors.

PR Close #38576
2020-09-03 14:02:35 -07:00
Pete Bacon Darwin 1d8c5d88cd refactor(compiler): `element.sourceSpan` should span the `outerHTML` (#38581)
Previously, the `sourceSpan` and `startSourceSpan` were the same
object, which meant that you had the following situation:

```
element = <div>some content</div>
sourceSpan = <div>
startSourceSpan = <div>
endSourceSpan = </div>
```

This made `sourceSpan` redundant and meant that if you
wanted a span for the whole element including its content
and closing tag, it had to be computed.

Now `sourceSpan` is separated from `startSourceSpan`
resulting in:

```
element = <div>some content</div>
sourceSpan = <div>some content</div>
startSourceSpan = <div>
endSourceSpan = </div>
```

PR Close #38581
2020-09-02 14:47:31 -07:00
crisbeto f5a148b1b7 fix(compiler): incorrectly inferring namespace for HTML nodes inside SVG (#38477)
The HTML parser gets an element's namespace either from the tag name
(e.g. `<svg:rect>`) or from its parent element `<svg><rect></svg>`) which
breaks down when an element is inside of an SVG `foreignElement`,
because foreign elements allow nodes from a different namespace to be
inserted into an SVG.

These changes add another flag to the tag definitions which tells child
nodes whether to try to inherit their namespaces from their parents.
It also adds a definition for `foreignObject` with the new flag,
allowing elements placed inside it to infer their namespaces instead.

Fixes #37218.

PR Close #38477
2020-08-31 13:25:38 -07:00
Alan Agius 0fc44e0436 feat(compiler-cli): add support for TypeScript 4.0 (#38076)
With this change we add support for TypeScript 4.0

PR Close #38076
2020-08-24 13:06:59 -07:00
crisbeto e7da4040d6 fix(compiler-cli): adding references to const enums in runtime code (#38542)
We had a couple of places where we were assuming that if a particular
symbol has a value, then it will exist at runtime. This is true in most cases,
but it breaks down for `const` enums.

Fixes #38513.

PR Close #38542
2020-08-21 12:23:21 -07:00
Andrew Kushnir cb05c0102f fix(core): move generated i18n statements to the `consts` field of ComponentDef (#38404)
This commit updates the code to move generated i18n statements into the `consts` field of
ComponentDef to avoid invoking `$localize` function before component initialization (to better
support runtime translations) and also avoid problems with lazy-loading when i18n defs may not
be present in a chunk where it's referenced.

Prior to this change the i18n statements were generated at the top leve:

```
var I18N_0;
if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) {
    var MSG_X = goog.getMsg(“…”);
    I18N_0 = MSG_X;
} else {
    I18N_0 = $localize('...');
}

defineComponent({
    // ...
    template: function App_Template(rf, ctx) {
        i0.ɵɵi18n(2, I18N_0);
    }
});
```

This commit updates the logic to generate the following code instead:

```
defineComponent({
    // ...
    consts: function() {
        var I18N_0;
        if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) {
            var MSG_X = goog.getMsg(“…”);
            I18N_0 = MSG_X;
        } else {
            I18N_0 = $localize('...');
        }
        return [
            I18N_0
        ];
    },
    template: function App_Template(rf, ctx) {
        i0.ɵɵi18n(2, 0);
    }
});
```

Also note that i18n template instructions now refer to the `consts` array using an index
(similar to other template instructions).

PR Close #38404
2020-08-17 10:13:57 -07:00
Andrew Kushnir 5f90b64328 refactor(compiler): i18n compiler tests refactoring (#38404)
This commit refactors i18n compiler tests to avoid code duplication and simplify further maintenance and updates.

PR Close #38404
2020-08-17 10:13:55 -07:00
Andrew Scott 71138f6004 feat(compiler-cli): Add compiler option to report errors when assigning to restricted input fields (#38249)
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
2020-08-11 09:55:48 -07:00
JoostK fa0104017a refactor(compiler-cli): only use type constructors for directives with generic types (#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
2020-08-11 09:55:48 -07:00
JoostK 18098d38b8 fix(compiler-cli): avoid creating value expressions for symbols from type-only imports (#37912)
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
2020-08-11 09:53:25 -07:00
JoostK 9514fd9080 fix(compiler): evaluate safe navigation expressions in correct binding order (#37911)
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
2020-08-11 09:51:10 -07:00
JoostK 7525f3afc1 fix(compiler-cli): type-check inputs that include undefined when there's coercion members (#38273)
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
2020-08-06 15:21:02 -07:00
Doug Parker dca4443a8e fix(compiler-cli): mark eager `NgModuleFactory` construction as not side effectful (#38320)
Roll forward of #38147.

This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered
side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are
only needed for compatibility with View Engine. This results in top-level constructor calls, such as:

```typescript
export const FooNgFactory = new NgModuleFactory(Foo);
```

`NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is
never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all
its unused dependencies from being tree shaken, making Closure builds significantly larger than necessary.

The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure
compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused
`NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be
removed (if nothing else references it), thus tree shaking unused dependencies as expected.

The one notable edge case is for lazy loaded modules. Internally, lazy loading is done as a side effect when the lazy
script is evaluated. For Angular, this side effect is registering the `NgModule`. In Ivy this is done by the
`NgModuleFactory` constructor, so lazy loaded modules **cannot** have their top-level `NgModuleFactory` constructor
call tree shaken. We handle this case by looking for the `id` field on `@NgModule` annotations. All lazy loaded modules
include an `id`. When this `id` is found, the `NgModuleFactory` is generated **without** with `noSideEffects()` call,
so Closure will not tree shake it and the module will lazy-load correctly.

PR Close #38320
2020-08-06 09:02:16 -07:00
Charles Lyding 6f6102d8ad fix(compiler): add PURE annotation to getInheritedFactory calls (#38291)
Currently the `getInheritedFactory` function is implemented to allow
closure to remove the call if the base factory is unused.  However, this
method does not work with terser.  By adding the PURE annotation,
terser will also be able to remove the call when unused.

PR Close #38291
2020-07-30 16:53:52 -07:00
Alex Rickabaugh 3a525d196b Revert "fix(compiler): mark `NgModuleFactory` construction as not side effectful (#38147)" (#38303)
This reverts commit 7f8c2225f2.

This commit caused test failures internally, which were traced back to the
optimizer removing NgModuleFactory constructor calls when those calls caused
side-effectful registration of NgModules by their ids.

PR Close #38303
2020-07-30 12:19:35 -07:00
Doug Parker 7f8c2225f2 fix(compiler): mark `NgModuleFactory` construction as not side effectful (#38147)
This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered
side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are
only needed for compatibility with View Engine. This results in top-level constructor calls, such as:

```typescript
export const FooNgFactory = new NgModuleFactory(Foo);
```

`NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is
never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all
its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making
Closure builds significantly larger than they should be.

The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure
compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused
`NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be
removed (if nothing else references it), thus tree shaking unused components as expected.

PR Close #38147
2020-07-29 13:32:08 -07:00
Doug Parker 887c350f9d refactor(compiler): wrap large strings in function (#38253)
Large strings constants are now wrapped in a function which is called whenever used. This works around a unique
limitation of Closure, where it will **always** inline string literals at **every** usage, regardless of how large the
string literal is or how many times it is used.The workaround is to use a function rather than a string literal.
Closure has differently inlining semantics for functions, where it will check the length of the function and the number
of times it is used before choosing to inline it. By using a function, `ngtsc` makes Closure more conservative about
inlining large strings, and avoids blowing up the bundle size.This optimization is only used if the constant is a large
string. A wrapping function is not included for other use cases, since it would just increase the bundle size and add
unnecessary runtime performance overhead.

PR Close #38253
2020-07-29 13:31:03 -07:00
Andrea Canciani 9c8bc4a239 fix(common): narrow `NgIf` context variables in template type checker (#36627)
When the `NgIf` directive is used in a template, its context variables
can be used to capture the bound value. This is sometimes used in
complex expressions, where the resulting value is captured in a
context variable. There's two syntax forms available:

1. Binding to `NgIfContext.ngIf` using the `as` syntax:
```html
<span *ngIf="enabled && user as u">{{u.name}}</span>
```

2. Binding to `NgIfContext.$implicit` using the `let` syntax:
```html
<span *ngIf="enabled && user; let u">{{u.name}}</span>
```

Because of the semantics of `ngIf`, it is known that the captured
context variable is truthy, however the template type checker
would not consider them as such and still report errors when
`strict` is enabled.

This commit updates `NgIf`'s context guard to make the types of the
context variables truthy, avoiding the issue.

Based on https://github.com/angular/angular/pull/35125

PR Close #36627
2020-07-29 10:30:44 -07:00
Andrew Kushnir 8e5969bb52 fix(compiler): share identical stylesheets between components in the same file (#38213)
Prior to this commit, duplicated styles defined in multiple components in the same file were not
shared between components, thus causing extra payload size. This commit updates compiler logic to
use `ConstantPool` for the styles (while generating the `styles` array on component def), which
enables styles sharing when needed (when duplicates styles are present).

Resolves #38204.

PR Close #38213
2020-07-27 10:04:30 -07:00