Skip to content

Create external symbol factory reexports #29459

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
Closed
wants to merge 3 commits into from
Closed

Create external symbol factory reexports #29459

wants to merge 3 commits into from
+190 −109

Conversation

kyliau
Copy link
Contributor

@kyliau kyliau commented Mar 22, 2019 •
edited
Loading

Exposes and fixes bug in #29454

Upstream PR: #29449

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Sorry, something went wrong.

@kyliau kyliau requested review from a team as code owners March 22, 2019 04:06
@kyliau kyliau requested review from gregmagolan and alexeagle March 22, 2019 04:30
@kyliau kyliau force-pushed the createExternalSymbolFactoryReexports branch from 15b05cb to e086a9e Compare March 22, 2019 04:32
@kyliau kyliau force-pushed the createExternalSymbolFactoryReexports branch from e086a9e to 72a43b5 Compare March 22, 2019 04:40
gregmagolan
gregmagolan approved these changes Mar 22, 2019
View reviewed changes
Copy link
Contributor

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

👌

@kyliau kyliau added target: major This PR is targeted for the next major release area: bazel Issues related to the published `@angular/bazel` build rules labels Mar 22, 2019
@ngbot ngbot bot added this to the needsTriage milestone Mar 22, 2019
gregmagolan
gregmagolan reviewed Mar 22, 2019
View reviewed changes
integration/bazel/src/BUILD.bazel
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
# TODO(kyliau): These are not necessary. Bundle compiles fine without
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why that is the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are direct dependencies of @angular/router, and it seems all transitive deps of targets listed here are included in the compilation.
I think the right thing to do here is to list them all explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, it's any deps that has NodeModuleInfo provider

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should assume that all scripts come in from transitive runfiles and work to boil this list down to the minimal set that we have a good reason to import here

alexeagle
alexeagle approved these changes Mar 22, 2019
View reviewed changes
Copy link
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.

awesome!!

integration/bazel/src/BUILD.bazel
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
# TODO(kyliau): These are not necessary. Bundle compiles fine without
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should assume that all scripts come in from transitive runfiles and work to boil this list down to the minimal set that we have a good reason to import here

@kyliau kyliau added the action: merge The PR is ready for merge by the caretaker label Mar 22, 2019
@mhevery mhevery closed this in 21be0fb Mar 22, 2019
mhevery pushed a commit that referenced this pull request Mar 22, 2019
@alexeagle @mhevery
fix(bazel): workaround problem reading summary files from node_modules (
769d960 
#29459)

PR Close #29459
mhevery pushed a commit that referenced this pull request Mar 22, 2019
@kyliau @mhevery
test(bazel): Add router to bazel integration test (#29459)
b5295ad 
PR Close #29459
@kyliau kyliau mentioned this pull request Mar 22, 2019
Closed
@kyliau kyliau deleted the createExternalSymbolFactoryReexports branch March 22, 2019 22:33
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@alexeagle @wKoza
fix(bazel): allow ng_module users to set createExternalSymbolFactoryR…
1630ea5 
…eexports (angular#29459)

PR Close angular#29459
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@alexeagle @wKoza
fix(bazel): workaround problem reading summary files from node_modules (
9f770bf 
angular#29459)

PR Close angular#29459
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@kyliau @wKoza
test(bazel): Add router to bazel integration test (angular#29459)
e8865d6 
PR Close angular#29459
@alexeagle alexeagle mentioned this pull request Apr 17, 2019
Closed
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gregmagolan gregmagolan

@alexeagle alexeagle

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kyliau @alexeagle @gregmagolan @googlebot