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

fix(ngcc): always add exports for `ModuleWithProviders` references #33875

Closed

Conversation

@JoostK
Copy link
Member

JoostK commented Nov 16, 2019

Please see individual commits for detailed descriptions.

Fixes #33701

Copy link
Member

petebacondarwin left a comment

First commit message typo:

as it may declare types that are only used internally

Second commit typo:

layouy

@JoostK JoostK force-pushed the JoostK:ngcc-private-modulewithproviders branch from e588b61 to adaf66c Nov 16, 2019
Copy link
Member

petebacondarwin left a comment

Third commit message: can you use format rather than bundle?

packages/compiler-cli/ngcc/test/integration/util.ts Outdated Show resolved Hide resolved
Copy link
Member

petebacondarwin left a comment

Great work (as always) @JoostK

A few nits with tests and commit messages but generally looks marvellous.

@JoostK JoostK marked this pull request as ready for review Nov 16, 2019
@JoostK JoostK requested a review from angular/fw-ngcc as a code owner Nov 16, 2019
@JoostK JoostK force-pushed the JoostK:ngcc-private-modulewithproviders branch 2 times, most recently from fdc223c to 54a66fa Nov 17, 2019
@JoostK JoostK force-pushed the JoostK:ngcc-private-modulewithproviders branch from 54a66fa to f217e3a Nov 17, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 18, 2019

@JoostK please remove the clean-up and add the merge labels when you are happy.

JoostK added 2 commits Nov 16, 2019
Some declaration files may not be referenced from an entry-point's
main typings file, as it may declare types that are only used internally.
ngcc has logic to include declaration files based on all source files,
to ensure internal declaration files are available.

For packages following APF layout, however, this logic was insufficient.
Consider an entry-point with base path of `/esm2015/testing` and typings
residing in `/testing`, the file
`/esm2015/testing/src/nested/internal.js` has its typings file at
`/testing/src/nested/internal.d.ts`. Previously, the declaration was
assumed to be located at `/esm2015/testing/testing/internal.d.ts` (by
means of `/esm2015/testing/src/nested/../../testing/internal.d.ts`)
which is not where the declaration file can be found. This commit
resolves the issue by looking in the correct directory.
ngcc has a basic integration test infrastructure that downlevels
TypeScript code into bundle formats that need to be processed by ngcc.
Until now, only ES5 bundles were created with a flat structure, however
more complex scenarios require an APF-like layout containing multiple
bundle formats.
In #32902 a bug was supposedly fixed where internal classes as used
within `ModuleWithProviders` are publicly exported, even when the
typings file already contained the generic type on the
`ModuleWithProviders`. This fix turns out to have been incomplete, as
the `ModuleWithProviders` analysis is not done when not processing the
typings files.

The effect of this bug is that formats that are processed after the
initial format had been processed would not have exports for internal
symbols, resulting in "export '...' was not found in '...'" errors.

This commit fixes the bug by always running the `ModuleWithProviders`
analyzer. An integration test has been added that would fail prior to
this change.

Fixes #33701
@JoostK JoostK force-pushed the JoostK:ngcc-private-modulewithproviders branch from f217e3a to d1049b0 Nov 18, 2019
alxhub added a commit that referenced this pull request Nov 18, 2019
Some declaration files may not be referenced from an entry-point's
main typings file, as it may declare types that are only used internally.
ngcc has logic to include declaration files based on all source files,
to ensure internal declaration files are available.

For packages following APF layout, however, this logic was insufficient.
Consider an entry-point with base path of `/esm2015/testing` and typings
residing in `/testing`, the file
`/esm2015/testing/src/nested/internal.js` has its typings file at
`/testing/src/nested/internal.d.ts`. Previously, the declaration was
assumed to be located at `/esm2015/testing/testing/internal.d.ts` (by
means of `/esm2015/testing/src/nested/../../testing/internal.d.ts`)
which is not where the declaration file can be found. This commit
resolves the issue by looking in the correct directory.

PR Close #33875
alxhub added a commit that referenced this pull request Nov 18, 2019
…33875)

ngcc has a basic integration test infrastructure that downlevels
TypeScript code into bundle formats that need to be processed by ngcc.
Until now, only ES5 bundles were created with a flat structure, however
more complex scenarios require an APF-like layout containing multiple
bundle formats.

PR Close #33875
alxhub added a commit that referenced this pull request Nov 18, 2019
…33875)

In #32902 a bug was supposedly fixed where internal classes as used
within `ModuleWithProviders` are publicly exported, even when the
typings file already contained the generic type on the
`ModuleWithProviders`. This fix turns out to have been incomplete, as
the `ModuleWithProviders` analysis is not done when not processing the
typings files.

The effect of this bug is that formats that are processed after the
initial format had been processed would not have exports for internal
symbols, resulting in "export '...' was not found in '...'" errors.

This commit fixes the bug by always running the `ModuleWithProviders`
analyzer. An integration test has been added that would fail prior to
this change.

Fixes #33701

PR Close #33875
@alxhub alxhub closed this in 985cadb Nov 18, 2019
alxhub added a commit that referenced this pull request Nov 18, 2019
…33875)

ngcc has a basic integration test infrastructure that downlevels
TypeScript code into bundle formats that need to be processed by ngcc.
Until now, only ES5 bundles were created with a flat structure, however
more complex scenarios require an APF-like layout containing multiple
bundle formats.

PR Close #33875
alxhub added a commit that referenced this pull request Nov 18, 2019
…33875)

In #32902 a bug was supposedly fixed where internal classes as used
within `ModuleWithProviders` are publicly exported, even when the
typings file already contained the generic type on the
`ModuleWithProviders`. This fix turns out to have been incomplete, as
the `ModuleWithProviders` analysis is not done when not processing the
typings files.

The effect of this bug is that formats that are processed after the
initial format had been processed would not have exports for internal
symbols, resulting in "export '...' was not found in '...'" errors.

This commit fixes the bug by always running the `ModuleWithProviders`
analyzer. An integration test has been added that would fail prior to
this change.

Fixes #33701

PR Close #33875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.