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

fix(zone.js): fesm2015 bundle should also be strict module. #40456

Closed
wants to merge 2 commits into from

Conversation

JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Jan 16, 2021

Close #40215

fesm2015/zone.js is built to esm bundle with rollup, so the 'use strict';
statement is not generated in the bundle, even we put the 'use strict' in the src code,
rollup removes the code in the final bundle.

So if we load the fesm2015/zone.js as a module, such as ng serve, in the index.html

<script src="polyfills.js" type="module"></script>

Everything works fine, since polyfills.js is loaded as module, so it is always strict.

But in ng test, webpack concat the zone.js and loaded into the karma html. For other app and
test code, they are still strict since they are module because they have export/import
statement, but zone.js is a bundle without export, it is a side effect bundle, so after
loaded by webpack, it becomes non-strict. Which causes some issues, such as #40215,
the root cause is the this context should be undefined but treated as Window in non-strict mode.

Object.prototype.toString.apply(undefined);
// should be [object undefined], but it is [object Window] in non-strict mode.

So this commit always add 'use strict'; statement at the beginning of the zone.js es2015 bundle.

@google-cla google-cla bot added the cla: yes label Jan 16, 2021
@pullapprove pullapprove bot requested a review from gkalpak January 16, 2021 01:24
@ngbot ngbot bot added this to the Backlog milestone Jan 16, 2021
@JiaLiPassion JiaLiPassion added the target: minor This PR is targeted for the next minor release label Jan 16, 2021
Copy link
Contributor

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

LGTM.

Although I think it's simpler to just add it as part of the banner.

* @license Angular v${version}

The fact that rollup is removing the use strict pragma makes sense since ES modules are strict by default.

However the problem here is that when used with Webpack this is causing an issue. Webpack will typically add the use-strict pragma to ES modules, however Webpack is unable to determine that zone.js is fact an ES module because it doesn't contain any expressions such as import or export statements.

Copy link
Contributor

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

LGTM.

Although I think it's simpler to just add it as part of the banner.

* @license Angular v${version}

The fact that rollup is removing the use strict pragma makes sense since ES modules are strict by default.

However the problem here is that when used with Webpack this is causing an issue. Webpack will typically add the use-strict pragma to ES modules, however Webpack is unable to determine that zone.js is fact an ES module because it doesn't contain any expressions such as import or export statements.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM for dev-infra.
I can't really speak about the implementation, but @alan-agius4's suggestion of putting it in the banner sounds reasonable (and less "magic") to me.

Reviewed-for: dev-infra

packages/zone.js/test/npm_package/npm_package.spec.ts Outdated Show resolved Hide resolved
@gkalpak gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 18, 2021
@JiaLiPassion
Copy link
Contributor Author

@alan-agius4, @gkalpak , thanks for the review. I have updated the PR.
Adding use strict to the license banner is much simpler, but it has another issue that is the es5 bundle will have double use strict generated.

such as bundles/zone.js will look like

'use strict';
/**
* @license Angular v<unknown>
* (c) 2010-2020 Google LLC. https://angular.io/
* License: MIT
*/
(function (factory) {
    typeof define === 'function' && define.amd ? define(factory) :
        factory();
}((function () {
    'use strict';

Which should not be a real issue.

@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 18, 2021
@JiaLiPassion JiaLiPassion force-pushed the use-strict branch 2 times, most recently from ff09427 to 0a775c8 Compare January 18, 2021 22:47
Close angular#40215

`fesm2015/zone.js` is built to `esm` bundle with rollup, so the 'use strict';
statement is not generated in the bundle, even we put the 'use strict' in the src code,
rollup removes the code in the final bundle.

So if we load the `fesm2015/zone.js` as a module, such as `ng serve`, in the index.html

```
<script src="polyfills.js" type="module"></script>
```

Everything works fine, since polyfills.js is loaded as `module`, so it is always `strict`.

But in `ng test`, webpack concat the `zone.js` and loaded into the karma html. For other app and
 test code, they are still `strict` since they are `module` because they have `export/import`
statement, but `zone.js` is a bundle without `export`, it is a `side effect` bundle, so after
 loaded by webpack, it becomes non-strict. Which causes some issues, such as angular#40215,
the root cause is the `this` context should be `undefined` but treated as `Window` in `non-strict` mode.

```
Object.prototype.toString.apply(undefined);
// should be [object undefined], but it is [object Window] in non-strict mode.

// zone.js patched version of toString
Object.prototype.toString = function() {
  ...
  // in non-strict mode, this is Window
  return originalObjectPrototypeToString.call(this);
}

```

So in this commit, `'use strict';` is always added to the `esm` bundles.
packages/zone.js/rollup-es5.config.js Outdated Show resolved Hide resolved
packages/zone.js/rollup-es5_global-es2015.config.js Outdated Show resolved Hide resolved
packages/zone.js/rollup-es5.config.js Show resolved Hide resolved
@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 Feb 19, 2021
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: zones cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ngOnDestroy handler crashes after upgrade to Angular 11 on 'ng test' (zone.js issue)
3 participants