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

add polyfills for ie11 #4642

Merged
merged 2 commits into from
Mar 14, 2021
Merged

Conversation

outofambit
Copy link
Contributor

@outofambit outofambit commented Jun 18, 2020

Resolves #4588

Changes:

Added additional polyfills to support IE11. This likely increases the bundle size, but I haven't looked at how much yet.
Related: babel/babel-preset-env#203
Reference: https://babeljs.io/docs/en/next/babel-preset-env.html#usebuiltins

To do:

  • verify bundle size hasn't skyrocketed
  • verify this fixes IE11

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

@limzykenneth
Copy link
Member

Are we doing tree-shaking to remove unused code? Likely that will make sure we don't include polyfills in core-js that we don't need.

@outofambit
Copy link
Contributor Author

Are we doing tree-shaking to remove unused code? Likely that will make sure we don't include polyfills in core-js that we don't need.

@limzykenneth 👍 we're not tree-shaking generally, but the babel-preset-env will transform the the core-js import line to only includes imports for features that we use in the codebase. (see https://babeljs.io/docs/en/next/babel-preset-env.html#usebuiltins-entry for an example of this)

@hydrosquall
Copy link
Contributor

[ ] verify bundle size hasn't skyrocketed

This might be a larger, separate task, but we could potentially use a service like bundlewatch automatically on every PR.

https://github.com/bundlewatch/bundlewatch

This would require someone with admin access to processing granting permissions to https://service.bundlewatch.io/setup-github .

Alternately, projects like material-ui implemented a bot using a Dangerfile (same tool used by both React GatsbyJS) mui/material-ui#14587 to run a similar check without introducing a dependency on the Bundlewatch hosted service.

@outofambit
Copy link
Contributor Author

outofambit commented Jan 23, 2021

Hey all, apologies for the long delay here. I ran the math and found the bundle size increase.

Filename Before After Size Change Percentage Change
p5.js 3.5 MB 4 MB +0.5 MB +14%
p5.min.js 604 KB 792 KB +188 KB +31%

Its kinda substantial (relatively speaking) so I'm not sure if we should merge this or formally make the decision not to support IE11. It looks like IE11 is currently 0.2% of all web traffic, but I'm not familiar enough with current education spaces to know if it's a common browser for educators to be tied to.

Then again, if you have 3G connection speed (2-3 Mbit/s) this would mean the download time for the minified version would go from ~2s to ~3s which maybe isn't that bad?

@limzykenneth do you have any thoughts here? I think you have a better sense for bundle sizes than me.

cc @stevenraysimon @lmccart @mcturner1995 in case any of y'all have thoughts too!

@limzykenneth
Copy link
Member

Ideally we would be able to support IE11 if possible, I guess one thing to really think about is for areas in the world with less access to bandwidth or new updates to hardware and software, which one is a bigger concern.

The polyfill seems to be adding more code than I expect it to if the result is selective (or tree shaked). I may try building this locally to look at the diff. Potentially we can manually optimize the polyfill to match our use case without bring in so much code.

@hydrosquall
Copy link
Contributor

hydrosquall commented Jan 25, 2021 via email

@limzykenneth
Copy link
Member

@hydrosquall That's an interesting idea though I don't think the solutions outlined in the article necessarily solves our problem

  1. polyfill.io simply adds more code that needs to be downloaded which seems like going in a different direction
  2. we have no control over the <script> tag that the user uses so that won't work as well
  3. I might be wrong but I think we are already using Babel's useBuiltIns? @outofambit

Maybe something like dynamic import (again itself polyfilled) could work though I'd prefer to explore this option only as a last resort, ideally we can reduce the size of the codebase in the first place as it can be expected those who would be using older browser like IE11 would probably have a significant overlap with people who have lower bandwidth limits.

@outofambit
Copy link
Contributor Author

I might be wrong but I think we are already using Babel's useBuiltIns?

@limzykenneth yep this PR adds config to use Babel's useBuiltIns option, however, i looked at the docs again and saw a better way to configure it that only includes the polyfills we need. this saved some space and i updated the table above! (saved .2MB on the full build, and 52KB on the minified one!) thanks for the poke! :D

@limzykenneth
Copy link
Member

I've had a look at the diff, and some of the polyfills although are for features present in IE11 but missing in browsers like Opera Mini or Android browser, dropping support for those would decrease final size more than dropping support for IE I think. There doesn't seem to be much that we can further optimize otherwise, one thing that can maybe help is to serve the files themselves with gzip compression which may help reduce file size by a good amount because of some repetitive elements in the polyfills, though it's not something that we can control from the library itself.

Overall I think it's fine to add these for now and maybe keep open the possibility of dropping some support for those browsers that are no longer being supported widely or considering optional inclusion of polyfill with feature detection and async code loading.

@outofambit outofambit marked this pull request as ready for review February 19, 2021 21:22
@lmccart
Copy link
Member

lmccart commented Mar 10, 2021

this looks great! just confirming, is this ready to merge @outofambit?

@outofambit
Copy link
Contributor Author

outofambit commented Mar 14, 2021

@lmccart it is now! i hadn't had a chance (or actual way) to test this with an IE11 browser to verify it fixes the original bug but i just figured out a way to do it and i did and looks like we're all good!

@outofambit outofambit merged commit 6fe2d80 into processing:main Mar 14, 2021
@outofambit outofambit deleted the fix-ie11-support branch March 14, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can there be a version of p5.js that works for Internet Explorer 11 (IE 11)?
4 participants