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

test(ngcc): fix rootDir issue and update ngcc integration test #34212

Closed
wants to merge 2 commits into from

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Dec 3, 2019

This probably replaces #33592

@gkalpak
gkalpak approved these changes Dec 3, 2019
Copy link
Member

gkalpak left a comment

Nice catch! I've banged my head for a couple of hours against the wall over this 😃
Can you expand a bit more on this part in the commit message:

This can result in errors when ngcc is trying to compile package formats after the typings files have already been processed.

(Why is it only a problem if typings have already been processed.)

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-cdk branch from 136ef12 to cc948aa Dec 4, 2019
@petebacondarwin petebacondarwin requested a review from IgorMinar Dec 4, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Dec 4, 2019

@gkalpak - I have expanded :-)

Previously the `rootDir` was set to the entry-point path but
this is incorrect if the source files are stored in a directory outside
the entry-point path. This is the case in the latest versions of the
Angular CDK.

Instead the `rootDir` should be the containing package path, which is
guaranteed to include all the source for the entry-point.

---

A symptom of this is an error when ngcc is trying to process the source of
an entry-point format after the entry-point's typings have already been
processed by a previous processing run.

During processing the `_toR3Reference()` function gets called which in turn
makes a call to `ReflectionHost.getDtsDeclaration()`. If the typings files
are also being processed this returns the node from the dts typings files.

But if we have already processed the typings files and are now processing
only an entry-point format without typings, the call to
`ReflectionHost.getDtsDeclaration()` returns `null`.

When this value is `null`, a JS `valueRef` is passed through as the DTS
`typeRef` to the `ReferenceEmitter`. In this case, the `ReferenceEmitter`
fails during `emit()` because no `ReferenceEmitStrategy` is able to provide
an emission:

1) The `LocalIdentifierStrategy` is not able help because in this case
`ImportMode` is `ForceNewImport`.
2) The `LogicalProjectStrategy` cannot find the JS file below the `rootDir`.

The second strategy failure is fixed by this PR.

Fixes angular/ngcc-validation#495
This commit updates `@angular/material` and `@angular/cdk` to the
latest release candidate. Doing so exposed a bug in ngcc, which is fixed
by the preceding commit. Also the layout of these libraries changed, so
the checks in the integration test need a bit of tweaking.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-cdk branch from cc948aa to 63cd7b3 Dec 4, 2019
@mhevery
mhevery approved these changes Dec 5, 2019
@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Dec 5, 2019

merge-assistance: Global approval

@petebacondarwin petebacondarwin removed the request for review from IgorMinar Dec 5, 2019
AndrewKushnir added a commit that referenced this pull request Dec 5, 2019
This commit updates `@angular/material` and `@angular/cdk` to the
latest release candidate. Doing so exposed a bug in ngcc, which is fixed
by the preceding commit. Also the layout of these libraries changed, so
the checks in the integration test need a bit of tweaking.

PR Close #34212
AndrewKushnir added a commit that referenced this pull request Dec 5, 2019
Previously the `rootDir` was set to the entry-point path but
this is incorrect if the source files are stored in a directory outside
the entry-point path. This is the case in the latest versions of the
Angular CDK.

Instead the `rootDir` should be the containing package path, which is
guaranteed to include all the source for the entry-point.

---

A symptom of this is an error when ngcc is trying to process the source of
an entry-point format after the entry-point's typings have already been
processed by a previous processing run.

During processing the `_toR3Reference()` function gets called which in turn
makes a call to `ReflectionHost.getDtsDeclaration()`. If the typings files
are also being processed this returns the node from the dts typings files.

But if we have already processed the typings files and are now processing
only an entry-point format without typings, the call to
`ReflectionHost.getDtsDeclaration()` returns `null`.

When this value is `null`, a JS `valueRef` is passed through as the DTS
`typeRef` to the `ReferenceEmitter`. In this case, the `ReferenceEmitter`
fails during `emit()` because no `ReferenceEmitStrategy` is able to provide
an emission:

1) The `LocalIdentifierStrategy` is not able help because in this case
`ImportMode` is `ForceNewImport`.
2) The `LogicalProjectStrategy` cannot find the JS file below the `rootDir`.

The second strategy failure is fixed by this PR.

Fixes angular/ngcc-validation#495

PR Close #34212
AndrewKushnir added a commit that referenced this pull request Dec 5, 2019
This commit updates `@angular/material` and `@angular/cdk` to the
latest release candidate. Doing so exposed a bug in ngcc, which is fixed
by the preceding commit. Also the layout of these libraries changed, so
the checks in the integration test need a bit of tweaking.

PR Close #34212
@petebacondarwin petebacondarwin deleted the petebacondarwin:ngcc-cdk branch Dec 7, 2019
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
…34212)

Previously the `rootDir` was set to the entry-point path but
this is incorrect if the source files are stored in a directory outside
the entry-point path. This is the case in the latest versions of the
Angular CDK.

Instead the `rootDir` should be the containing package path, which is
guaranteed to include all the source for the entry-point.

---

A symptom of this is an error when ngcc is trying to process the source of
an entry-point format after the entry-point's typings have already been
processed by a previous processing run.

During processing the `_toR3Reference()` function gets called which in turn
makes a call to `ReflectionHost.getDtsDeclaration()`. If the typings files
are also being processed this returns the node from the dts typings files.

But if we have already processed the typings files and are now processing
only an entry-point format without typings, the call to
`ReflectionHost.getDtsDeclaration()` returns `null`.

When this value is `null`, a JS `valueRef` is passed through as the DTS
`typeRef` to the `ReferenceEmitter`. In this case, the `ReferenceEmitter`
fails during `emit()` because no `ReferenceEmitStrategy` is able to provide
an emission:

1) The `LocalIdentifierStrategy` is not able help because in this case
`ImportMode` is `ForceNewImport`.
2) The `LogicalProjectStrategy` cannot find the JS file below the `rootDir`.

The second strategy failure is fixed by this PR.

Fixes angular/ngcc-validation#495

PR Close angular#34212
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
This commit updates `@angular/material` and `@angular/cdk` to the
latest release candidate. Doing so exposed a bug in ngcc, which is fixed
by the preceding commit. Also the layout of these libraries changed, so
the checks in the integration test need a bit of tweaking.

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

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 7, 2020

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 Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.