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 Constructor WeakMap requires 'new' #51

Closed
wants to merge 1 commit into from
Closed

Fix Constructor WeakMap requires 'new' #51

wants to merge 1 commit into from

Conversation

zhouzi
Copy link

@zhouzi zhouzi commented Aug 12, 2017

When using this polyfill in Chrome, I'm getting this error:

Constructor WeakMap requires 'new'

After some google searches, it seems that extending built-in is not supported. This PR adds a babel plugin allowing that.

Also fix #14

EDIT: Chrome version is 60.0.3112.90, OS is macOS Sierra 10.12.5

Gruntfile.js Outdated
.replace(/\n$/, "")
.replace(/^[^{]/gm, " $&");
.replace(/\n$/, "")
.replace(/^[^{]/gm, " $&");
Copy link
Member

Choose a reason for hiding this comment

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

Could you undo the change to these two lines?

@zhouzi
Copy link
Author

zhouzi commented Aug 21, 2017

@Rob--W I just reverted the changes to the mentioned lines.

@Rob--W
Copy link
Member

Rob--W commented Aug 21, 2017

Could you squash all commits too? Otherwise git blame will still unnecessarily point to these commits.

@zhouzi
Copy link
Author

zhouzi commented Aug 22, 2017

Here you go, all in one 😄

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

LGTM
@rpl This can be merged.

@Rob--W
Copy link
Member

Rob--W commented Aug 22, 2017

Wait, the build script of webextension-polyfill itself does not transpile the source to ES5, so the class DefaultWeakMap extends WeakMap syntax should work without any issues in Chrome.

If we used a transpiler, then this patch would be good to go. However, we don't transpile, so why should this PR be merged?

@zhouzi
Copy link
Author

zhouzi commented Aug 22, 2017

I double checked and in fact it works as expected as long as the code is not transpiled. The error comes from the transpiled version of the code (using babel). Which means the issue is not much of webextension-polyfill's business. Sorry for the mistake!

@zhouzi zhouzi closed this Aug 22, 2017
@zhouzi zhouzi deleted the extend-weakmap branch August 22, 2017 18:20
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.

ESLint causes build on windows to fail with default git settings
2 participants