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

Differential Loading via Bundle Downleveling #15246

Merged
merged 4 commits into from
Aug 8, 2019

Conversation

clydin
Copy link
Member

@clydin clydin commented Aug 5, 2019

This change removes the requirement to perform two full builds of the application to create a differential loaded application. Instead only the ES2015 application is built and then the resulting application bundles are then downleveled directly. This has the advantage of reducing the build time as well as a small size improvement in the ES5 bundles. Lazy loaded bundles are also consistently named between the ES2015 and ES5 bundles. Speed improvements range from 10% to 50% with larger applications benefiting the most.

The existing behavior is currently retained via the use of the NG_BUILD_DIFFERENTIAL_FULL environment variable. The existing behavior is available at this point in time to ensure that any unknown problematic edge cases within the new build pipeline will not cause existing applications to fail to build.

@clydin clydin added the target: major This PR is targeted for the next major release label Aug 5, 2019
@clydin clydin requested a review from alan-agius4 August 5, 2019 01:12
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Mainly NITS.

The main issues that I saw were;

  1. Hidden sourcemaps are not handled as a reference is always added in the bundle.
  2. vendorSourceMaps seem to broken as well for ES5.
  3. Scripts are not being optimised for ES5 which might cause them to break on IE. (left some more info in one of the above comments)
  4. Maybe show a message that something is happening, as it's quite misleading to not to have the terminal available after the stats have been emitted.

@clydin clydin force-pushed the bundle-downlevel-2 branch 2 times, most recently from fbd239c to 94017ae Compare August 5, 2019 17:12
Closes angular#13228
It is currently unused and requires an old version of the `source-map` package.  This old version conflicts with the use of newer versions that are required to provide the necessary functionality and performance within the published packages.
…e-map version (0.7.3)

The latest version provides significant performance improvements.
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Really happy with this! Well done!

@JiaLiPassion
Copy link
Contributor

@alan-agius4 , this one need to wait for angular/angular#32016, because currently loading zone-legacy.js before zone-evergreen.js will throw error, this bug has been fixed in the above PR. thanks!

@clydin
Copy link
Member Author

clydin commented Aug 8, 2019

@JiaLiPassion All the browser tests are passing without that PR. When is this error occurring?

Chrome 41, Safari 9, Firefox 48, IE 10, IE 11, and Edge 14 are all tested and are using zone legacy.

@JiaLiPassion
Copy link
Contributor

@clydin, this error should occur when loading zone-legacy.js without loading zone-evergreen.js because zone-legacy.js will call Zone.__symbol__ at the beginning, so in this PR, the zone-legacy.js will be loaded at the very beginning, is that correct?

@clydin
Copy link
Member Author

clydin commented Aug 8, 2019

It's loaded before evergreen but they are bundled together. Just tried it locally and there is no error. Also added a console log statement inside the legacy registerElement patch function and it was executed.

@clydin
Copy link
Member Author

clydin commented Aug 8, 2019

Also, looking at the code for 0.9.1, there shouldn't be an issue there. Zone isn't accessed until the function is executed due to the use of var Zone = _global["Zone"];

         (function(_global) {
          _global["__zone_symbol__legacyPatch"] = function() {
            var Zone = _global["Zone"];
            Zone.__load_patch("registerElement", function(global, Zone, api) {
              registerElementPatch(global, api);
              console.log('hello from zone legacy');
            });
            Zone.__load_patch("EventTargetLegacy", function(global, Zone, api) {
              eventTargetLegacyPatch(global, api);
              propertyDescriptorLegacyPatch(api, global);
            });
          };
        })(
          (typeof window !== "undefined" && window) ||
            (typeof self !== "undefined" && self) ||
            global
        );

@JiaLiPassion
Copy link
Contributor

@clydin, oh, got it, now the repo is still using old version of zone.js which doesn't have the problem, the new version of zone.js (0.10.1) have the error I described, sorry for the unclear information, this PR is ok with old version of zone.js. Thanks.

@clydin
Copy link
Member Author

clydin commented Aug 8, 2019

No problem. Thanks for the info on the latest version though.

The CLI cannot use anything past 0.9.x until @angular/core updates its peer dependency. The CLI has a blocked PR for the zone.js version update: #15179

@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants