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.

Sign up for GitHub

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

Jump to bottom

fix(ngcc): several ngcc fixes and test improvements #44245

Closed
wants to merge 7 commits into from
Closed

fix(ngcc): several ngcc fixes and test improvements #44245

wants to merge 7 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Nov 21, 2021

See individual commits for details.

Fixes #44019.

@gkalpak gkalpak added type: bug/fix state: WIP target: patch This PR is targeted for the next patch release comp: ngcc labels Nov 21, 2021
@google-cla google-cla bot added the cla: yes label Nov 21, 2021
@ngbot ngbot bot modified the milestone: Backlog Nov 21, 2021
@mary-poppins
Copy link

You can preview 8e4d73c at https://pr44245-8e4d73c.ngbuilds.io/.

@mary-poppins
Copy link

You can preview b4e0495 at https://pr44245-b4e0495.ngbuilds.io/.

@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Nov 22, 2021
@mary-poppins
Copy link

You can preview b6a1732 at https://pr44245-b6a1732.ngbuilds.io/.

@gkalpak gkalpak added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 22, 2021
@mary-poppins
Copy link

You can preview bae78d7 at https://pr44245-bae78d7.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review November 22, 2021 20:18
@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Nov 22, 2021
@gkalpak gkalpak requested review from petebacondarwin and JoostK and removed request for JoostK November 22, 2021 20:18
@gkalpak gkalpak mentioned this pull request Nov 22, 2021
Closed
15 tasks
gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Nov 22, 2021
@gkalpak
Try PR angular/angular#44245
c3f2bb3
JoostK
JoostK approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extensive test updates! 💯

packages/compiler-cli/ngcc/src/host/umd_host.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/host/umd_host.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/host/umd_host.ts Show resolved Hide resolved
packages/compiler-cli/ngcc/test/dependencies/umd_dependency_host_spec.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/test/helpers/umd_utils.ts Show resolved Hide resolved
packages/compiler-cli/ngcc/test/helpers/umd_utils.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts Outdated Show resolved Hide resolved
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
refactor(ngcc): make it easy to support more UMD wrapper function for…
d1d7a80 
…mats (#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup. However, this failed to account for a different format, using
`if/else` statements, such as the one [emitted by Webpack][1].

This commit prepares ngcc for supporting different UMD wrapper function
formats by decoupling the operation of parsing the wrapper function body
to capture the various factory function calls and that of operating on
the factory function calls (for example, to read or update their
arguments). In a subsequent commit, this will be used to add support for
the Webpack format.

[1]: https://webpack.js.org/configuration/output/#type-umd

PR Close #44245
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
fix(ngcc): support the UMD wrapper function format emitted by Webpack (…
bf5f734 
…#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup.

This commit adds support for a different format, that uses `if/else`
statements, which is what is [emitted by Webpack][1].

[1]: https://webpack.js.org/configuration/output/#type-umd

Fixes #44019

PR Close #44245
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
test(ngcc): use helper function for generating UMD modules for tests (#…
5daa34f 
…44245)

Previously, several ngcc test suites used their own helper to generate
test UMD modules.

This commit switches to using the same helper for generating UMD modules
across test suites. This improves DRYness (ensuring changes/fixes to the
UMD format need only be applied once) and makes it easier to test
different UMD formats in all test suites.

PR Close #44245
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
test(ngcc): run more tests against more supported UMD formats (#44245)
41562a5 
This commit utilizes the infrastructure added in the previous commit to
run more tests against more of the supported UMD formats. This shall
give us more confidence that all aspects of UMD processing work
correctly with the various formats.

PR Close #44245
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
fix(ngcc): correctly report error when collecting dependencies of UMD…
b68994d 
… module (#44245)

Previously, the ngcc `UmdReflectionHost` would throw a misleading error
when trying to collect dependencies of an invalidly formatted UMD
module. This happened because an error would be thrown while trying to
construct the error message for the actual error, by calling `getText()`
on certain TypeScript AST nodes. See
#44019 (comment)
for a more in-depth explanation.

This commit ensures `getText()` can be safely called on TypeScript AST
nodes when collecting dependencies of UMD modules.

PR Close #44245
dylhunn pushed a commit that referenced this pull request Nov 29, 2021
@gkalpak @dylhunn
test(ngcc): use main property in test package.json files for UMD …
1d0b54a 
…tests (#44245)

Previously, the mock packages created for `UmdDependencyHost`'s tests,
specified the entry-point as `esm2015`. This does not matter in tests,
since the packages are explicitly passed to the `UmdDependencyHost`
(while in reality the appropriate host would be determined based on the
name of the entry-point property - in this case, detecting the
entry-point as ES2015 and not UMD).

However, in order to avoid confusion, this commit updates the test
packages to use `main` (the default property used for the UMD format in
`package.json` files).

PR Close #44245
@gkalpak gkalpak deleted the fix-ngcc-webpack-umd branch November 29, 2021 19:27
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
refactor(ngcc): make it easy to support more UMD wrapper function for…
2e31f92 
…mats (angular#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup. However, this failed to account for a different format, using
`if/else` statements, such as the one [emitted by Webpack][1].

This commit prepares ngcc for supporting different UMD wrapper function
formats by decoupling the operation of parsing the wrapper function body
to capture the various factory function calls and that of operating on
the factory function calls (for example, to read or update their
arguments). In a subsequent commit, this will be used to add support for
the Webpack format.

[1]: https://webpack.js.org/configuration/output/#type-umd

PR Close angular#44245
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
fix(ngcc): support the UMD wrapper function format emitted by Webpack (…
fc07293 
…angular#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup.

This commit adds support for a different format, that uses `if/else`
statements, which is what is [emitted by Webpack][1].

[1]: https://webpack.js.org/configuration/output/#type-umd

Fixes angular#44019

PR Close angular#44245
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
test(ngcc): use helper function for generating UMD modules for tests (a…
6b1a69e 
…ngular#44245)

Previously, several ngcc test suites used their own helper to generate
test UMD modules.

This commit switches to using the same helper for generating UMD modules
across test suites. This improves DRYness (ensuring changes/fixes to the
UMD format need only be applied once) and makes it easier to test
different UMD formats in all test suites.

PR Close angular#44245
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
test(ngcc): run more tests against more supported UMD formats (angula…
ee855b8 
…r#44245)

This commit utilizes the infrastructure added in the previous commit to
run more tests against more of the supported UMD formats. This shall
give us more confidence that all aspects of UMD processing work
correctly with the various formats.

PR Close angular#44245
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
fix(ngcc): correctly report error when collecting dependencies of UMD…
a8be244 
… module (angular#44245)

Previously, the ngcc `UmdReflectionHost` would throw a misleading error
when trying to collect dependencies of an invalidly formatted UMD
module. This happened because an error would be thrown while trying to
construct the error message for the actual error, by calling `getText()`
on certain TypeScript AST nodes. See
angular#44019 (comment)
for a more in-depth explanation.

This commit ensures `getText()` can be safely called on TypeScript AST
nodes when collecting dependencies of UMD modules.

PR Close angular#44245
gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 30, 2021
@gkalpak
test(ngcc): use main property in test package.json files for UMD …
a1bd9d7 
…tests (angular#44245)

Previously, the mock packages created for `UmdDependencyHost`'s tests,
specified the entry-point as `esm2015`. This does not matter in tests,
since the packages are explicitly passed to the `UmdDependencyHost`
(while in reality the appropriate host would be determined based on the
name of the entry-point property - in this case, detecting the
entry-point as ES2015 and not UMD).

However, in order to avoid confusion, this commit updates the test
packages to use `main` (the default property used for the UMD format in
`package.json` files).

PR Close angular#44245
@gkalpak gkalpak mentioned this pull request Nov 30, 2021
Merged
gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 5, 2021
@gkalpak
fix(ngcc): correctly resolve UMD dependencies
86d5c2e 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in angular#44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes angular#44380
@gkalpak gkalpak mentioned this pull request Dec 5, 2021
Closed
gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 5, 2021
@gkalpak
fix(ngcc): correctly resolve UMD dependencies
40459d9 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in angular#44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes angular#44380
@gkalpak gkalpak mentioned this pull request Dec 5, 2021
Closed
alxhub pushed a commit that referenced this pull request Dec 7, 2021
@gkalpak @alxhub
fix(ngcc): correctly resolve UMD dependencies (#44381)
4126591 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in #44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes #44380

PR Close #44381
alxhub pushed a commit that referenced this pull request Dec 7, 2021
@gkalpak @alxhub
fix(ngcc): correctly resolve UMD dependencies (#44381)
2062225 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in #44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes #44380

PR Close #44381
alxhub pushed a commit that referenced this pull request Dec 7, 2021
@gkalpak @alxhub
fix(ngcc): correctly resolve UMD dependencies (#44382)
b6554d7 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in #44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes #44380

PR Close #44382
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
refactor(ngcc): make it easy to support more UMD wrapper function for…
d98ecda 
…mats (angular#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup. However, this failed to account for a different format, using
`if/else` statements, such as the one [emitted by Webpack][1].

This commit prepares ngcc for supporting different UMD wrapper function
formats by decoupling the operation of parsing the wrapper function body
to capture the various factory function calls and that of operating on
the factory function calls (for example, to read or update their
arguments). In a subsequent commit, this will be used to add support for
the Webpack format.

[1]: https://webpack.js.org/configuration/output/#type-umd

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
fix(ngcc): support the UMD wrapper function format emitted by Webpack (…
1a515be 
…angular#44245)

Previously, ngcc could only handle UMD modules whose wrapper function
was implemented as a `ts.ConditionalExpression` (i.e. using a ternary
operator). This is the format emitted by popular bundlers, such as
Rollup.

This commit adds support for a different format, that uses `if/else`
statements, which is what is [emitted by Webpack][1].

[1]: https://webpack.js.org/configuration/output/#type-umd

Fixes angular#44019

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
test(ngcc): use helper function for generating UMD modules for tests (a…
2a7c49f 
…ngular#44245)

Previously, several ngcc test suites used their own helper to generate
test UMD modules.

This commit switches to using the same helper for generating UMD modules
across test suites. This improves DRYness (ensuring changes/fixes to the
UMD format need only be applied once) and makes it easier to test
different UMD formats in all test suites.

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
test(ngcc): run more tests against more supported UMD formats (angula…
7b79071 
…r#44245)

This commit utilizes the infrastructure added in the previous commit to
run more tests against more of the supported UMD formats. This shall
give us more confidence that all aspects of UMD processing work
correctly with the various formats.

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
fix(ngcc): correctly report error when collecting dependencies of UMD…
fb89525 
… module (angular#44245)

Previously, the ngcc `UmdReflectionHost` would throw a misleading error
when trying to collect dependencies of an invalidly formatted UMD
module. This happened because an error would be thrown while trying to
construct the error message for the actual error, by calling `getText()`
on certain TypeScript AST nodes. See
angular#44019 (comment)
for a more in-depth explanation.

This commit ensures `getText()` can be safely called on TypeScript AST
nodes when collecting dependencies of UMD modules.

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
test(ngcc): use main property in test package.json files for UMD …
c8bf18e 
…tests (angular#44245)

Previously, the mock packages created for `UmdDependencyHost`'s tests,
specified the entry-point as `esm2015`. This does not matter in tests,
since the packages are explicitly passed to the `UmdDependencyHost`
(while in reality the appropriate host would be determined based on the
name of the entry-point property - in this case, detecting the
entry-point as ES2015 and not UMD).

However, in order to avoid confusion, this commit updates the test
packages to use `main` (the default property used for the UMD format in
`package.json` files).

PR Close angular#44245
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@gkalpak @dimakuba
fix(ngcc): correctly resolve UMD dependencies (angular#44381)
83bf943 
Previously, when processing UMD, ngcc assumed that the `exports`
argument of the CommonJS factory call (if present) would be the first
argument of the call. This is generally true for the supported UMD
formats, but can change if ngcc prepends more imports (and thus factory
arguments) while processing the module. This could lead to errors when
trying to collect dependencies of an already processed module.
(This was accidentally broken in angular#44245 (commit 2bc3522).)

This commit fixes it by not making any assumptions about the position of
an `exports` argument in the CommonJS factory call.

Fixes angular#44380

PR Close angular#44381
@angular-automatic-lock-bot
Copy link

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 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@JoostK JoostK JoostK approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'text' of undefined - Error: NGCC failed.
5 participants
@gkalpak @mary-poppins @JoostK @dylhunn @petebacondarwin