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

I825 no additional entry points #839

Merged
merged 1 commit into from
Jul 31, 2019
Merged

I825 no additional entry points #839

merged 1 commit into from
Jul 31, 2019

Conversation

Jesse-Good
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x ] Tests for the changes have been added (for bug fixes / features)
  • [x ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • [ x] Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

rollup_bundle build rule requires additional_entry_points explicitly listed.

Issue Number: 825

What is the new behavior?

rollup_bundle adds boolean parameter auto_additional_entry_points, which can be used instead of additional_entry_points. auto_additional_entry_points is True by default.

Does this PR introduce a breaking change?

  • [ x] Yes
  • No

Existing invocations of rollup_bundle() that omit additional_entry_points may need to set auto_additional_entry_points to False.

Other information

alexeagle
alexeagle reviewed Jun 10, 2019
View reviewed changes
internal/e2e/rollup_code_splitting/BUILD.bazel Show resolved Hide resolved
internal/web_package/test/BUILD.bazel Outdated
@@ -5,6 +5,7 @@ package(default_visibility = ["//visibility:public"])

rollup_bundle(
name = "bundle",
auto_additional_entry_points = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

in particular I think we want to have it True here - I'd like to understand if that's breaking by changing the tests to illustrate just what that breakage is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//internal/web_package/test2:test fails because the <script> tags look different. There's a bunch of <script type="module" and nomodule in the actual output but the test is not expecting that.
test.log

Copy link
Collaborator

Choose a reason for hiding this comment

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

paired on this in person - we now understand that the golden file changes to the HTML are desirable - enabling code splitting had the side effect of enabling differential loading

alexeagle
alexeagle reviewed Jun 10, 2019
View reviewed changes
Copy link
Collaborator

@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.

The new test doesn't demonstrate that code splitting is happening - are there actually additional chunks produced?

@Jesse-Good
Copy link
Contributor Author

The new test doesn't demonstrate that code splitting is happening - are there actually additional chunks produced?

Yes, chunk is being produced but with name chunk-X.js. Is it safe to write a test against the hashed X value?

alexeagle
alexeagle reviewed Jun 20, 2019
View reviewed changes
Copy link
Collaborator

@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.

LGTM after adding a new test case for additional entry points (since the existing e2e will be used for code splitting under a single entry point)

internal/rollup/rollup_bundle.bzl Outdated
@@ -611,10 +611,19 @@ ROLLUP_ATTRS = {
It is sufficient to load one of these SystemJS boilerplate/entry point
files as a script in your HTML to load your application""",
),
"auto_additional_entry_points": attr.bool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

decided to rename it "enable_code_splitting" since this is orthogonal to the number of entry points

internal/rollup/rollup_bundle.bzl Outdated Show resolved Hide resolved
@Jesse-Good
Copy link
Contributor Author

PTAL, merged from origin and responded to comments.

@alexeagle alexeagle force-pushed the i825-noAdditionalEntryPoints branch 2 times, most recently from dc0133c to d72ac10 Compare July 16, 2019 02:59
@alexeagle alexeagle force-pushed the i825-noAdditionalEntryPoints branch 5 times, most recently from 3903a34 to f829e78 Compare July 31, 2019 22:35
gregmagolan
gregmagolan approved these changes Jul 31, 2019
View reviewed changes
Copy link
Collaborator

@gregmagolan gregmagolan left a comment •
edited

Choose a reason for hiding this comment

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

LGTM.
It may be useful to manually try this out in angular-bazel-example and see if you can remove additional_entry_points there and test that it still lazy load the routes before landing.

internal/rollup/rollup.config.js
@@ -206,17 +202,16 @@ const config = {
preserveSymlinks: true,
}

if (enableCodeSplitting) {
config.experimentalCodeSplitting = true;
config.experimentalDynamicImport = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. Looks like the feature is no longer experimental since I worked on this 👍

internal/e2e/rollup_code_splitting/BUILD.bazel Outdated
additional_entry_points = ["build_bazel_rules_nodejs/internal/e2e/rollup_code_splitting/additional_entry.js"],
enable_code_splitting = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would implied to be true because additional_entry_points is specified?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this should have been an error, will fix

@Jesse-Good @alexeagle
feat(builtin): do code splitting even if only one entry point
583889b 
This separates handling of multiple entry points from code splitting
@alexeagle alexeagle force-pushed the i825-noAdditionalEntryPoints branch from f829e78 to 583889b Compare July 31, 2019 23:01
@alexeagle
Copy link
Collaborator

I'll try it in angular-bazel-example before next release - want to get Jesse's PR in under his authorship before I do more with it

@alexeagle alexeagle merged commit f51c129 into bazelbuild:master Jul 31, 2019
@mgechev mgechev mentioned this pull request Aug 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeagle alexeagle alexeagle left review comments

@gregmagolan gregmagolan gregmagolan approved these changes

@kyliau kyliau Awaiting requested review from kyliau

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Jesse-Good @alexeagle @gregmagolan @googlebot