Skip to content

fix: add mappings for ngfactory & ngsummary files to module names in aot summary resolver#25335

Closed
gregmagolan wants to merge 1 commit intoangular:masterfrom
gregmagolan:amd-module-name-for-ngfactory
Closed

fix: add mappings for ngfactory & ngsummary files to module names in aot summary resolver#25335
gregmagolan wants to merge 1 commit intoangular:masterfrom
gregmagolan:amd-module-name-for-ngfactory

Conversation

@gregmagolan
Copy link
Copy Markdown
Contributor

@gregmagolan gregmagolan commented Aug 6, 2018

When building angular from source using bazel downstream, the ngfactory & ngsummary files generated in the angular build also need to be mapped to their module names.

This PR makes bazelbuild/rules_typescript#223 unnecessary.

Copy link
Copy Markdown
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

In a follow-up PR, please convert integration/bazel (or make a new integration/bazel_src) that tests this

@alexeagle alexeagle added the target: patch This PR is targeted for the next patch release label Aug 6, 2018
@mary-poppins
Copy link
Copy Markdown

You can preview bb3e905 at https://pr25335-bb3e905.ngbuilds.io/.

@gregmagolan gregmagolan changed the title fix: add mappings for ngfactory files to module name in aot summary resolver fix: add mappings for ngfactory & ngsummary files to module names in aot summary resolver Aug 6, 2018
@mary-poppins
Copy link
Copy Markdown

You can preview 48198f5 at https://pr25335-48198f5.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview 041b2e9 at https://pr25335-041b2e9.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview 3d371f9 at https://pr25335-3d371f9.ngbuilds.io/.

@kara kara added the area: core Issues related to the framework runtime label Aug 6, 2018
@gregmagolan gregmagolan added the action: merge The PR is ready for merge by the caretaker label Aug 7, 2018
@gregmagolan gregmagolan requested a review from vicb August 7, 2018 02:16
kara pushed a commit that referenced this pull request Aug 7, 2018
@kara kara closed this in 02e201a Aug 7, 2018
matsko pushed a commit that referenced this pull request Aug 23, 2018
… ngfactory & ngsummary files (#25604)

Workaround was added in #25335. It was necessary for .ngfactory & .ngsummary files to have proper AMD module names starting with @angular when building angular downstream from source using Bazel. The underlying issue has been resolved in the compiler and these files now get proper AMD module names without the need for this workaround. The workaround had an unexpected consequence angular/angular-cli#11835 which is fixed by its removal.

PR Close #25604
matsko pushed a commit that referenced this pull request Aug 23, 2018
… ngfactory & ngsummary files (#25604)

Workaround was added in #25335. It was necessary for .ngfactory & .ngsummary files to have proper AMD module names starting with @angular when building angular downstream from source using Bazel. The underlying issue has been resolved in the compiler and these files now get proper AMD module names without the need for this workaround. The workaround had an unexpected consequence angular/angular-cli#11835 which is fixed by its removal.

PR Close #25604
@angular-automatic-lock-bot
Copy link
Copy Markdown

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

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants