No longer compatible with ES5

[sulliwane]

Hello,

I'm using this package in my create-react-app, and when I run npm run build I get this error:

Failed to minify the code from this file: 
 	./node_modules/save-svg-as-png/saveSvgAsPng.js:3 
Read more here: http://bit.ly/2tRViJ9

Could you confirm your package is not ES5 compatible?
Thank you!

[exupero]

Thanks for the bug report. I don't see any reason this library wouldn't be compatible with ES5. That bit.ly link doesn't suggest a problem with the actual source code, nor with ES5 compatibility. The Try Babel site is able to transpile the current version of the script without any problems (see this link).

Can you supply a more specific error that indicates what the problem is?

[Jephuff]

The problem is likely that create-react-app doesn't run node_modules through babel.
A work around to this is to override create-react-app to transpile specific modules using react-app-rewired
with a config-overrides.js file that has something like

const path = require('path');
const fs = require('fs');

const appDirectory = fs.realpathSync(process.cwd());
const resolveApp = relativePath => path.resolve(appDirectory, relativePath);
module.exports = function(config) {
  config.module.rules[1].oneOf[1].include = [
    config.module.rules[1].oneOf[1].include,
    resolveApp('node_modules/save-svg-as-png'),
  ];
  return config;
}

[sulliwane]

@exupero to reproduce the error:

npx create-react-app my-app
cd my-app
import saveSVGmodule from 'save-svg-as-png';  # this is a line to add to react app project
npm run build  # this step will fail when trying to minmize

I suspect the problem comes from the arrow functions, that are not part of ES5 (but part of ECMASCRIPT 2015 / ES6).

As @Jephuff suggested, one workaround could be to either "eject" CRA, or "rewire" it.

Or to offer this module in two flavours, the default one that should be "ES5" compliant (transpiled to ES5). And a ES6+ flavour for people who want to import it...

@exupero would you consider making a transpiled to ES5 version the default one? And adding a esnext key to the package.json for those who want to enjoy ES6+ ?

here is some lecture on the topic:

• https://github.com/bookercodes/articles/blob/master/how-to-build-and-publish-es6-npm-modules-today-with-babel.md
• http://2ality.com/2017/06/pkg-esnext.html

thanks!

[exupero]

I'm hesitant to include a second, ES5 version of this library as a checked-in file in the repo. Having to build and check in a separate version makes it more difficult for contributors, who now have to distinguish which file should be edited, as well as building and checking in the transpiled file. It also creates potential for the two files to get out of sync. ES6 is widely supported natively, and if something in your build pipeline requires ES5, I tend to think it's your responsibility to include the proper ES6 transpilation step. It sounds like there are common solutions to handling this in your app.

[MilosRasic]

We're getting an error in runtime in the same line in IE 11 exclusively, which I assume doesn't support arrow functions.

@exupero Publishing completely transpiled ES5 code as "main" file is a common practice in browser javascript open source. "module" key is used these days (last I checked at least) to direct bundlers to an ES6 build which should be compatible with most modern browsers.

If you're making a decision to not support ES5, which is still strange, at least make it in a major version release. This way you break the internet.

[exupero]

Can you supply more information about the error and line of code? I'm not able to test in IE 11, but this ES6 compatibility chart indicates that IE 11 should support ES6 arrow functions.

Thanks for the additional info about supplying two versions of the code. I'd happily look at a pull request to see what's involved.

[MilosRasic]

idk man, I see a solid red with 0/13

I'm unable to test as well atm, but our Bugsnag caught simply "syntax error" from many IE 11 users. Here's a sample of device data from one of the errors:

browserName: IE
browserVersion: 11.0.0
locale: en-US
osName: Windows 7
userAgent: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; .NET4.0C; .NET4.0E; InfoPath.3; rv:11.0) like Gecko

[exupero]

You're right. That compatibility chart is bunk. It's showing different results in different browsers, which I don't think should happen for any column other than "Current Browser".

I'll work on restoring ES5 compatibility.

[exupero]

I've created #173 to fix ES5 compatibility. Test it out and let me know if you run into any problems.

[MilosRasic]

Just tested with IE 11 from a Windows machine. Confirmed the problem with the latest version. Tested the fix/es5-compatibility branch with npm link and everything works fine.

While it's definitely working with the fix, I'm not sure babel-polyfill should be made a dependency. Most project using babel will already be including it, either through import or bundler config. What happens if a project uses babel 7's @babel/polyfill package? Duplicate polyfills in the bundle probably. Not sure if there would be any conflicts.

There's a pretty good discussion about it at w3ctag/polyfills#6

[exupero]

Thanks for the feedback. I removed babel-polyfill.

[sulliwane]

the new transpiled to ES5 version works for me as well. Thanks a lot, and looking forward the NPM release 💯

[exupero]

Published v1.4.3 to NPM. Please re-open if you encounter any problems.