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 the wrapping IIFE of the prod bundle #208

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Fix the wrapping IIFE of the prod bundle #208

merged 1 commit into from
Jun 5, 2019

Conversation

mgechev
Copy link
Contributor

@mgechev mgechev commented May 28, 2019

Currently, the semantics of line 59 in Gruntfile.js is - expose the library as a global called true. This causes two problems:

  1. The wrapping mechanism of uglify breaks in strict mode (more below)
  2. There's a leaking global variable called true

The used version of uglify wraps the code in the following construct:

"(function(" + parameters.join(",") + "){ '$ORIG'; })(" + args.join(",") + ")";

The IIFE passes the global this to a function and later tries to set the property name that we've assigned to wrap. This works fine in non-strict cases. The only side effect is the global window.true that we set to an empty object. In strict mode, however, global this equals to undefined. We try to set undefined['true'] which cases a runtime error.

We noticed the above scenario after enabling differential loading with script[type="module"], which uses strict mode by default.

It'll be fantastic if we can release the fix today, so we don't block the CLI release.

Side note, the wrap function of uglify is fixed for strict mode here. Maybe update of grunt and grunt-contrib-uglify is worth it.

Currently, the semantics of [line 59 in `Gruntfile.js`](https://github.com/web-animations/web-animations-js/blob/ae52749433415fe979396bb82e7e5a0bdf9a115a/Gruntfile.js#L59) is - expose the library as a global called `true`. This causes two problems:

1. The wrapping mechanism of uglify breaks in strict mode (more below)
2. There's a leaking global variable called `true`

The used version of uglify wraps the code in the following construct:

```js
"(function(" + parameters.join(",") + "){ '$ORIG'; })(" + args.join(",") + ")";
```

The IIFE passes the global `this` to a function and later tries to set the property name that we've assigned to `wrap`. This works fine in non-strict cases. The only side effect is the global `window.true` that we set to an empty object. In strict mode, however, global `this` equals to `undefined`. We try to set `undefined['true']` which cases a runtime error.

We noticed the above scenario after enabling differential loading with `script[type="module"]`, which uses strict mode by default.

It'll be fantastic if we can release the fix today, so we don't block the CLI release.

Side note, the wrap function of uglify is fixed for strict mode [here](https://github.com/mishoo/UglifyJS2/pull/1811/files#diff-da84828c4bd7834c0040b0bbb51acdec). Maybe update of `grunt` and `grunt-contrib-uglify` is worth it.
@alexeagle
Copy link

Note, Angular 8 is shipping today and this fix is critical for us, so we'll direct our users to have an npm dependency on web-animations-js@github:angular/web-animations-js#release_pr208 where we have prepared a custom release including this fix

Copy link
Contributor

@flackr flackr left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I've verified this still exposes the expected interfaces in the minified bundles as well as working with module imports. I'm unfortunately unable to verify on the test suite due to an unrelated failure.

@viceice
Copy link

viceice commented May 29, 2019

@flackr Maybe a simple travis retry? Looks like a temporary git problem.

@alancutter
Copy link
Contributor

Looks like Travis is having trouble pulling the w3c repositories it uses to run tests. I don't think it's been run in a long time. I can probably find some cycles to get it running again after branch point.

@alexeagle
Copy link

Could we merge and release this even without the green travis? The current release of Angular is using a fork of this project to work around the problem, we'd like to get off our fork before too many users depend on it.

@fbecquier
Copy link

fbecquier commented Jun 4, 2019

Could we try to relaunch a build?

@flackr
Copy link
Contributor

flackr commented Jun 5, 2019

I'm not sure how to fix the travis errors, but having tested manually I'll go ahead and merge this.

@flackr flackr merged commit 5cad16b into web-animations:dev Jun 5, 2019
@mgechev mgechev deleted the minko/fix-prod-bundle branch June 6, 2019 11:26
@mgechev
Copy link
Contributor Author

mgechev commented Jun 6, 2019

Do you think we can release?

@alan-agius4
Copy link

@flackr, can we please get a release for this? Thanks

@dscalzi
Copy link

dscalzi commented Jun 20, 2019

Bump

@kdinev
Copy link

kdinev commented Jun 21, 2019

Any chance this can get released any time soon?

@alancutter
Copy link
Contributor

Published 2.3.2 including this change, please report if you come across any issues with the new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants