Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

do not leak polyfills into global context; closes #4423 #4446

Closed
wants to merge 5 commits into from

Conversation

boneskull
Copy link
Contributor

when we transitioned to Rollup/Babel, we misconfigured Babel such that all language APIs polyfilled by core-js would be accessible not just by Mocha, but by tests and code under test.

This change attempts to "restrict" the polyfills to Mocha itself.

I'm not entirely sure the best way to test this, since our tests also get bundled and would thus have access to our polyfills. We need to basically add a separate script to a Karma config which would get loaded via <script> tag to make any assertions about the global environment. I am too lazy to do this rn.

Will it work with IE? Let's find out.

Bonus: 50k bundle size reduction!

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Sep 14, 2020
@boneskull boneskull self-assigned this Sep 14, 2020
@coveralls
Copy link

coveralls commented Sep 14, 2020

Coverage Status

Coverage remained the same at 93.959% when pulling c190c40 on boneskull/issue/4423-polyfill-leak into 738a575 on master.

@boneskull
Copy link
Contributor Author

still misconfigured... i hate javascript

@boneskull
Copy link
Contributor Author

yeah, I am not sure how to do this. using the "runtime" babel helper, doesn't make sense, because everything must be bundled (no externals). using "bundled" pollutes the globals.

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@Munter
Copy link
Contributor

Munter commented Sep 18, 2020

Ugh. Babel is clearly not built for this. It's possible we should try buble instead. That only handles syntax though. Then we'd have to curate our polyfills ourselves. But it seems preferable to the magic that's keeping us from getting the output we want right now

@boneskull
Copy link
Contributor Author

I think I could curate the polyfills w/o needing to swap out babel. it just seems incredibly weird that babel won't handle this for us.

afaict there are some movements in this direction (babel-polyfill-corejs3 for instance) but I haven't been able to get it working. seems experimental at this point, but I think that's the problem trying to be solved.

configuring babel is so much not fun. it is very low-level, and I think our use-case is unique enough to make something higher-level out of reach as wel

@JoshuaKGoldberg
Copy link
Member

Closing to keep our PR queue clean as there are a lot of merge conflicts. But we'll take another look at the underlying issue when we have time - reducing the size & clobbering-ness of polyfills is going to be a nice thing for us to do.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Mocha adds forEach property to CSSStyleDeclaration, at least inside a Proxy
4 participants