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

Transpile ES6 for smoother IE11 support #24

Closed
jalada opened this issue Apr 5, 2019 · 23 comments
Closed

Transpile ES6 for smoother IE11 support #24

jalada opened this issue Apr 5, 2019 · 23 comments

Comments

@jalada
Copy link

jalada commented Apr 5, 2019

Adding gatsby-background-image to a project causes issues in IE11 because it has ES6-syntax in its JS. Gatsby doesn't transpile third-party libraries (gatsbyjs/gatsby#12399 gatsbyjs/gatsby#3780).

You can use gatsby-plugin-compile-es6-packages but this can be error prone if the library also needs polyfills, because Gatsby won't automatically provide those either. It can be pretty challenging to know if a library needs a manual polyfill added.

I added this to my gatsby-config.js:

plugins: [
    {
      resolve: `gatsby-plugin-compile-es6-packages`,
      options: {
        modules: [`gatsby-background-image`]
      }
    }
]

And that does seem to have resolved the issue, but I think (and this is definitely personal opinion!) it would be better if the npm package for this library had the code already compiled.

I appreciate there's a wider issue around IE11 and the needed polyfills; so please close this if the automatic integration is being tracked elsewhere, so long as this is part of that :)

@timhagn
Copy link
Owner

timhagn commented Apr 5, 2019

Hi @jalada (&@DSchau), welcome to the party : )!

Thanks for bringing this up, have completely missed it, cause I was thinking I already were using the babel-transpile mechanism from gatsby-image through babel-preset-gatsby-package.
And as you might have gathered from some other issues, I only tested gatsby-background-image with IE11 on a Win 10 VM, and it didn't seem to raise a problem ^^.

But, fortunately, solving your issue was an easy one - at least it seems that way:
I forgot to add the { "browser": true } option to the babel-preset!
So of course most of the build was left in ES6+ % ).

Just published 0.2.9 and couldn't find any leftover const or suchlike in the transpiled build ; ).
Would you be so kind to test the new version without gatsby-plugin-es6-packages?

@JonathanAndrews
Copy link

@timhagn Just checked this, using the 0.2.9 version without gatsby-plugin-compile-es6-packages and I looked good to me 👍

@timhagn
Copy link
Owner

timhagn commented Apr 8, 2019

Thanks @JonathanAndrews for testing it - and am happy it worked : ).
Might I guess that you, @jalada, tested it, too (according to the thumbs up emoji ; )?
Should this issue be fixed, feel free to have the honors of closing it : ).

@jalada
Copy link
Author

jalada commented Apr 8, 2019

Will do! And yes indeed; @JonathanAndrews is on my team.

Thanks for your help Tim!

@jalada jalada closed this as completed Apr 8, 2019
@timhagn
Copy link
Owner

timhagn commented Apr 9, 2019

Was happy to help - and thanks to all participants for bringing this up,
might have led to future issues otherwise ; ).

@vhenzl
Copy link

vhenzl commented May 9, 2019

Is this really working? I had to use gatsby-plugin-compile-es6-packages in my project to prevent errors in IE11 (gatsby-background-image v0.5.5). timhagn/gbitest doesn't work without it either.

@timhagn
Copy link
Owner

timhagn commented May 9, 2019

Would you mind to share the type of errors (or warnings) you are receiving so other contributors and I will be able to replicate them (in my place on a VM)? A quick gatsby info would be of help to : ).
gatsby-background-image internally uses babel-preset-gatsby-package as a preset for babel transpiling with its browser option set to true (since this issue), so it shouldn't be a transpile problem...

@vhenzl
Copy link

vhenzl commented May 9, 2019

Thanks for your response. Here are my steps to reproduce:

  1. git clone [email protected]:timhagn/gbitest.git
  2. yarn && yarn start
  3. Open website in IE 11, it fails with "Object doesn't support property or method 'from'" error on Array.from(classes) in getStyle() in BackgroundUtils.js.

gatsby info:

  System:
    OS: Windows 10
  Binaries:
    Node: 10.15.3
    Yarn: 1.15.2
    npm: 6.4.1
  Languages:
    Python: 2.7.15
  Browsers:
    Edge: 44.17763.1.0
  npmPackages:
    gatsby: 2.4.2 => 2.4.2
    gatsby-background-image: ^0.5.5 => 0.5.5
    gatsby-image: ^2.0.41 => 2.0.41
    gatsby-plugin-manifest: 2.1.1 => 2.1.1
    gatsby-plugin-offline: ^2.1.0 => 2.1.0
    gatsby-plugin-react-helmet: 3.0.12 => 3.0.12
    gatsby-plugin-sharp: 2.0.36 => 2.0.36
    gatsby-plugin-styled-components: ^3.0.7 => 3.0.7
    gatsby-source-filesystem: ^2.0.33 => 2.0.33
    gatsby-transformer-sharp: ^2.1.19 => 2.1.19

IE version: 11.437.17763.0, update 11.0.120 (KB4493435)

I don't know how exactly babel-preset-gatsby-package works, but couldn't be a problem that it uses different settings in dev and production mode?

https://github.com/gatsbyjs/gatsby/blob/3779201/packages/babel-preset-gatsby-package/index.js#L12-L14

IE 11 is excluded in dev mode there.

If I run yarn build && yarn serve, I'm still getting some errors, though. In the minimises code is hard to tell where they come from, but it might not be gatsby-background-image.

@timhagn
Copy link
Owner

timhagn commented May 9, 2019

Hi @vhenzl,

thanks for your detailed setup, though I already could replicate some grave errors with v0.5.5 in IE11 this afternoon.

So I worked through some IE flaws once again (quite a throwback to the browser wars era ; ) and found the following problems running on a VM with nearly the same specs as yours:

  1. Same here with Array.from(), errorred out with the msg you mentioned.
    • SOLUTION: Added @babel/polyfill to dependencies, errors gone.
  2. I had previously removed setting imageRef.src to prevent duplicate loading of the base image on nearly all other browsers.
    • SOLUTION for IE11 (and probably Safari): I reintroduced it as an ultimate fallback without affecting other Browsers.

For the moment the just published v0.5.6 should rectify most of your issue's behavior, although the _tracedSVG fragment displays a faulty SVG size during dev mode and parsing of background-* CSS-props won't work through some strange quirks with "MSStyleCSSProperties" (style rules in IE).

I definitely can replicate your observation with yarn develop vs yarn serve. AFAIK babel-preset-gatsby-package shall get a revamp after Gatsby Days in NYC ^^.

Hope this works for you momentarily anyway (and that some day IE will be a thing of the past ; ).

Oh, and don't forget having a look at polyfilling IntersectionObserver like mentioned in the README.
And for gatsby-image add the object-fit / -position polyfill like described over at gatsby-image's README as well.

Best,
Tim.

EDIT: Added Info on IO & object-fit / -position polyfills.

@vhenzl
Copy link

vhenzl commented May 9, 2019

Thanks a lot for your work and detailed comment.

Unfortunately, Array.from() still doesn't work for me (in both my project and gbitest) – it's the same as before. It's not a big deal, I'll keep using gatsby-plugin-compile-es6-packages.

The problem with constructor.name and MSStyleCSSProperties ("Unknown style object prototype" error) was another thing I wanted to report, so thanks for fixing it. It now works fine.

@timhagn
Copy link
Owner

timhagn commented May 10, 2019

No prob, helped soothe an itch I had about currentSrc in some browsers ; ).
Though it's really strange, in my Win 10 VM with the same IE build version as yours, Array.from() gets handled without a problem by @babel/polyfill oO. Gonna dig a little more into Gatsby's babel configs, perhaps it sparks some light % ).
EDIT: Shot in the dark, but have you already tried deleting node_modules and running gatsby clean (if you shouldn't know this one, it deletes the .cache & public folders). Afterwards try yarn & yarn develop % ).

@timhagn
Copy link
Owner

timhagn commented May 12, 2019

Hi @vhenzl! I just published v0.5.7 transpiled with @babel/runtime-corejs3 instead of @babel/polyfill (as the latter seems to be deprecated for babel >= 7.4.0 % ). Looks like the polyfills are getting applied if necessary (especially at Array.from()), so would you mind testing it without gatsby-plugin-compile-es6-packages again? Though I have to say, as MSStyleCSSProperties doesn't seem to be iterable in IE11, background-* style parsing sadly doesn't work.

@vhenzl
Copy link

vhenzl commented May 12, 2019

Yes, v0.5.7 works without gatsby-plugin-compile-es6-packages, everything is now correctly polyfilled. Thanks a lot!

@timhagn
Copy link
Owner

timhagn commented May 12, 2019

No problem, you're welcome - should have rather read than only skimmed the babel docs way sooner (and might not only have caused some errors in IE, so thanks for pointing this out ; ). Should you need the background-* style parsing or the _tracedSVG Fragment's behavior, you'd have to blow a whistle and I'd more than welcome if you could investigate this further : ). IE isn't the main target browser of gbi, as you might have guessed, but of course I'd like it to work without a problem on all the "main" ones ^^.

@DSchau
Copy link

DSchau commented May 13, 2019

Just a heads up here that including babel-polyfill spikes the library size a bit here (about tripled).

[email protected] -> 5.5Kb (gzipped)
[email protected] -> 14.1Kb (gzipped)

Here's the why of what is happening.

We use the usage setting of babel-preset-env, which polyfills based on the features our code uses (e.g. if we use Set, set is polyfilled, if we use Array.from, that is polyfilled, etc.). However, because node_modules are not run through babel, vendored code is not polyfilled like we'd expect (thus Array.from is not a function in IE 11).

So we could go a few ways here:

  1. Transpile node_modules with babel in gatsby (heavy, but works; could be a theoretical breaking change)
  2. Load the polyfill (as was done here, however this spikes the library size for just IE 11)
  3. Use a different mechanism for Array.from functionality, e.g. Array.prototype.slice.call
    // both are arrays
    const uno = Array.prototype.slice.call(document.querySelectorAll('ul')) 
    const dos = Array.from(document.querySelectorAll('ul'))

I'd recommend option #3 here, since the code is about the same and the library size will be fairly minimal.

@timhagn
Copy link
Owner

timhagn commented May 13, 2019

Hi @DSchau,

thanks a lot for this concise explanation and presenting possible solutions all at once : )!
Completely forgot to check the bundle size before publishing (I just wanted IE to be gone again from my plate ; ). Of course you are right, tripling the size for only a few selected browsers isn't the way to go.
Gonna look into it : )!

Thanks again, best,

Tim.
EDIT: Mkay, @babel/runtime-corejs3 seems a little aggressive, as it wanted to add a polyfill for Array.slice while transpiling the source files oO. Gonna revert to the default babel-preset-gatsby-package % ).

@timhagn
Copy link
Owner

timhagn commented May 13, 2019

Hi @DSchau,

followed your guidance and replaced Array.from with Array.prototype.slice.call and the following .find with its Array.reduce equivalent (at least according to caniuse.com it's not available in IE 11, too). Forgot to change @babel/runtime-corejs3 back to @babel/runtime before publishing v0.5.8 - so we are now at v0.5.9 % ).
package-size now tells me:

 package                        size      minified  gzipped
 [email protected]  88.88 KB  16.47 KB  5.38 KB

(bundlephobia hasn't updated its cache at the time of writing ; ).
@vhenzl would you mind testing v0.5.9 with IE one more time?

Thanks and best wishes,

Tim.

@vhenzl
Copy link

vhenzl commented May 13, 2019

Well… v0.5.9 works. Right now. By coincidence. But it will break sooner or later again.

It isn't, and never been, about Array.from only. It's about ES6+ generally. If you look at the bundled code in the package, you will see that it contains other ES6+ features, which aren't polyffiled or transpiled, namely Object.values: https://unpkg.com/[email protected]/BackgroundUtils.js

It works right now thanks to Object.values (and possibly other ES6+ code) being polyfilled by a Gatsby's code. Particularly, .cache/error-overlay-handler.js contains

const errors = Object.values(errorMap)

which ensures what polyfill for Object.values is added to commons.js:

// around line 1470 for timhagn/gbitest project:

/***/ "./.cache/error-overlay-handler.js":
/*!*****************************************!*\
  !*** ./.cache/error-overlay-handler.js ***!
  \*****************************************/
/*! exports provided: clearError, reportError, errorMap */
//...
/* harmony import */ var core_js_modules_es7_object_values__WEBPACK_IMPORTED_MODULE_3__ = __webpack_require__(/*! core-js/modules/es7.object.values */ "./node_modules/core-js/modules/es7.object.values.js");
/* harmony import */ var core_js_modules_es7_object_values__WEBPACK_IMPORTED_MODULE_3___default = /*#__PURE__*/__webpack_require__.n(core_js_modules_es7_object_values__WEBPACK_IMPORTED_MODULE_3__);

However, if (or better say when) error-overlay-handler.js changes and Object.values gets removed from there, it will break again because it will not be "magically" polyfilled anymore. (You can simulate this by modifying the code in .cache to const errors = []).

As I see it, there are three options:

  1. To follow what done with Array.from and rewrite all other parts using ES6+ syntax to ES5 (i.e. no Object.values etc)
  2. To include all necessary polyfills in the package as was in v0.5.7.
  3. To say that this package targets ES6+ and advise in README to use gatsby-plugin-compile-es6-packages if necessary to target older browsers.

Since 1) would be maintenance hell and 2) unnecessary increases package size for those who target ES6+ only, option 3) is IMO the only reasonable way to go. It also makes sense from the long term view as Gatsby 3 will hopefully support compiling dependencies with babel-preset-env (gatsbyjs/gatsby#7064).

@vhenzl
Copy link

vhenzl commented May 13, 2019

Actually, my previous comment is valid only for development mode (yarn develop). In production mode (yarn build && yarn serve), v0.5.9 doesn't work. In IE 11 it results in "Object doesn't support property or method 'values'" error. Which is actually expected based on what I described above: error-overlay-handler.js is a utility for development which gets excluded in production build and therefore Object.values isn't polyfilled.

@timhagn
Copy link
Owner

timhagn commented May 13, 2019

*miff* That's a bummer : (.
But definitely thanks for your time spent dissecting this issue further, @vhenzl greatly appreciated!
And although I'm not happy with it in some way, I can only agree to your line of thought on this. It's probably best to "throw the towel" and advise on gatsby-plugin-compile-es6-packages. What's your opinion on this @DSchau? How's this handled with packages in the monorepo (am just getting into them)?
So thanks again! I'm gonna sleep over and dig into it ; ).

@DSchau
Copy link

DSchau commented May 13, 2019

I think the "burden" of using this (or rather, any) package should fall to the consumer of the package if they have specific browser requirements.

Generally - if you can manage it, I'd recommend avoiding using features that require a polyfill (e.g. Object.values, Array.from, for of loops, etc.) because the requirements for a library author are different than that of an application author (e.g. someone writing Gatsby code).

In the case of Object.values, a simplified solution (that would probably work here too) would be something like:

const values = obj => Object.keys(obj).map(key => obj[key])

It's not an ideal solution, but I'm not sure there's a better solution (again, apart from eventually transpiling node_modules in Gatsby--which we're not really quite ready to do yet!). I strongly encourage you to not bundle the polyfill, as that is imposing a triple-the-size package on all users for <=2.75% of the web.

@timhagn
Copy link
Owner

timhagn commented May 14, 2019

Hi @DSchau & @vhenzl!

As I mentioned yesterday, I was just getting into monorepos & lerna.
So I had an idea:
Why not publish a mini-monorepo for gatsby-background-image with split subpackages for a completely transpiled gatsby-background-image-es5 and the default gatsby-background-image.
Said, tried and done - but sadly botched my first lerna publish for gatsby-background-image-es5 (forgot to add @babel/runtime-corejs3 again % ).
So we now have to wait 24h before I can republish it -.-...

But it seems to work having a symlinked src in packages/gatsby-background-image-es5 with a .babelrc.js adapted to transpile to ES5 with polyfills. Thinking bout if it were possible to automatically add the intersection-observer polyfill to it, but don't know if this would present a dependency problem...

Any thoughts on, comments or even fixes to the monorepo's config and setup would be greatly appreciated : )!

@timhagn
Copy link
Owner

timhagn commented May 15, 2019

@vhenzl & @DSchau Now both are published at v0.6.1 : )!

  package                        size      minified  gzipped
  [email protected]  89.17 KB  16.5 KB   5.39 KB
  package                            size       minified  gzipped
  [email protected]  289.49 KB  42.89 KB  13.53 KB

Any thoughts are still appreciated! Perhaps @jalada or @JonathanAndrews would want to chime in?

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

No branches or pull requests

5 participants