Skip to content
This repository was archived by the owner on Sep 20, 2019. It is now read-only.

Move WeakMap polyfill to the beginning. #234

Closed

Conversation

threeday0905
Copy link
Contributor

  • webcomponents-lite.js - WeakMap polyfill may be executed before MutationObserver
  • webcomponent.js - Move WeakMap polyfill outside condition statement.

* webcomponents-lite.js - WeakMap polyfill may be executed before MutationObserver
* webcomponent.js - Move WeakMap polyfill outside condition statement.
@addyosmani
Copy link
Member

Thanks for your PR. Could you sign our CLA so we can move ahead with reviewing your changes?

Also: could you include some more details where the current build setup/positioning is causing breakage?

@garlicnation
Copy link
Contributor

This looks like it could be related to #210.

@threeday0905
Copy link
Contributor Author

Hi,

CLA has been signed.

I think this issue is same as #210.

In this pr, I just update build.json and build-lite.json to move WeakMap polyfill in front of the file, the reason as following.


In current webcomponents-lite.js build, WeakMap is polyfill after than MutationObserver. But MutationObserver is depend on WeakMap, like below:

(function(global) {
    var registrationsTable = new WeakMap();
    var setImmediate;


//.... after 300 lines ...

if (typeof WeakMap === "undefined") {
  (function() {

So if browser does not support WeakMap (like older version chrome, in china), it will cause some exception. And if swap WeakMap and MutationObserver, the code works good.


In webcomponents.js build, WeakMap polyfill is inside a shadow dom block like below:

if (WebComponents.flags.shadow) {
  if (typeof WeakMap === "undefined") {
      // code is here
  }
}

I think WeakMap polyfill and ShadowDom polyfill is independent module, so I update the build.json to move WeakMap outside ShadowDom polyfill.

@addyosmani
Copy link
Member

@paulie4 would this address any of the issues you were seeing?

@threeday0905
Copy link
Contributor Author

oops, I forgot to switch to my personal account. I will close this PR.

@paulie4
Copy link

paulie4 commented Mar 11, 2015

@addyosmani, yes it does, thanks @threeday0905!

BTW, was this PR already pulled into https://github.com/webcomponents/webcomponentsjs, cause I don't see that it has?

@threeday0905
Copy link
Contributor Author

@paulie4, actually I close this pr without merge. should I reopen this pr?

@paulie4
Copy link

paulie4 commented Mar 11, 2015

Why did you close the pull request?

@threeday0905
Copy link
Contributor Author

I use I am using the other email (not github account) to commit this change. So that I close this pr and revert that commit.

If it is ok, free feel to reopen this pr and merge it.

@threeday0905
Copy link
Contributor Author

@paulie4

I am using the other email (not github account) to commit this change. So that I close this pr and revert that commit.

If it is ok, free feel to reopen this pr and merge it.

@paulie4
Copy link

paulie4 commented Mar 11, 2015

I am not a contributor to this project, so someone else would need to merge this pull request (and reopen it, if necessary).

@saranrapjs
Copy link

This issue (in Safari 7, WeakMap being undefined) remains an issue. @threeday0905 are you planning on committing this with a different GitHub account? If not, does someone else need to create a new pull request with these changes?

@threeday0905
Copy link
Contributor Author

@saranrapjs, I had created another PR based on latest master branch, #240.

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

Successfully merging this pull request may close these issues.

5 participants