Skip to content
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

ngtsc tries to compile non-exported components while ngc did not #33724

Closed
devversion opened this issue Nov 11, 2019 · 4 comments
Closed

ngtsc tries to compile non-exported components while ngc did not #33724

devversion opened this issue Nov 11, 2019 · 4 comments

Comments

@devversion
Copy link
Member

@devversion devversion commented Nov 11, 2019

Affected package

@angular/compiler-cli ngtsc.

Problem description

Currently compiling a TS project which contains both test files and application code, NGC properly ignores non-exported components from test files.

When updating to v9, where ngtsc runs by default, the compilation will fail since ngtsc tries to compile the @Component annotations even for test components. Apparently since those test components are never part of a @NgModule, the compilation will fail (e.g. component not found).

accordion.spec.ts

@Component({
  template: `<cdk-accordion>..</cdk-accordion>`
})
/** notice: not exported */ class TestComponent {}

One could argue that test files and application code shouldn't be part of the same compilation unit, but this has been working in View Engine and helped speeding up test compilation. There are more benefits like not having to use path mapping inside tests. i.e. relative imports to application code work just fine.

Another benefit is that the application/library components are compiled AOT. This is the primary reason why we follow this pattern (in angular/components).

Note: CLI projects follows the same pattern where tests and application code are part of the same compilation unit. The CLI just runs tsc instead of the Angular compiler (which we want for AOT)

Possible solutions

  • Saying that this is the expected behavior, though it will be a change in v9 that involves infrastructure changes. It doesn't seem easy to split into two compilation units, if tests and application code are located next to each other.
  • Following NGC behavior by ignoring symbols which are not exported.
  • Adding jit: true to every test component.

Reproduction

https://github.com/devversion/ngtsc-test-component-analysis

@ngbot ngbot bot added this to the needsTriage milestone Nov 11, 2019
@IgorMinar IgorMinar modified the milestones: needsTriage, v9-candidates Nov 11, 2019
@IgorMinar IgorMinar added freq1: low and removed freq1: low labels Nov 19, 2019
@IgorMinar

This comment has been minimized.

Copy link
Member

@IgorMinar IgorMinar commented Nov 19, 2019

@devversion I'm a bit puzzled about why you are running ngc on the spec file. How does this speed up compilation?

@devversion

This comment has been minimized.

Copy link
Member Author

@devversion devversion commented Nov 19, 2019

@IgorMinar It speeds up compilation because we compile unit tests and application code within the same TypeScript compilation unit. If we'd split it into two compilation units, then we first build the application code, and have a second compilation unit for testing. The second compilation unit would start with a fresh ts.Program and would need to read parts of the previous compilation again.

It's generally common to build tests and application code in a same compilation unit. The CLI does the same by default, but just with the difference that it uses tsc while we want ngc. ngc will then process application code and AOT-compile it (e.g. generates JIT summaries), while the test code remains untouched since components are not exported (=> and this is different in ngtsc).

I realized that this is not quite what we do in angular/components since we do not actually use the JIT summaries, but this could be a general issue (not specific to Angular Components)

@devversion

This comment has been minimized.

Copy link
Member Author

@devversion devversion commented Nov 19, 2019

I think the frequency of this issue is very low because AOT unit testing is not a common thing in Angular yet. Though it can be a common thing with Ivy since AOT unit testing will be very easy then.

At that point, this issue will become relevant again and one of the possible solutions mentioned in this issue would need to be taken (unless we come up with something different).

TL;DR: It's a breaking change, but it's a rare one and there are two possible solutions. This issue will ultimately come up again in the future IMO when AOT unit testing with Ivy is a thing. I'm not sure if we should take any premature action yet. This feels like something we need to think more about (so that we can come up with a thought-through solution for Ivy)

crisbeto added a commit to crisbeto/angular that referenced this issue Nov 19, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 21, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 21, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 21, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 21, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 25, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.
@matsko matsko closed this in 25dcc76 Nov 25, 2019
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Dec 11, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes angular#33724.

PR Close angular#33921
AndrewKushnir added a commit that referenced this issue Dec 11, 2019
In ViewEngine we were only generating code for exported classes, however with Ivy we do it no matter whether the class has been exported or not. These changes add an extra flag that allows consumers to opt into the ViewEngine behavior. The flag works by treating non-exported classes as if they're set to `jit: true`.

Fixes #33724.

PR Close #33921

PR Close #34340
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 26, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.