Pomax / react-onclickoutside

An onClickOutside wrapper for React components
MIT License
1.82k stars 187 forks source link

Need transpiled JS or ES6 src code #185

Closed Dante-101 closed 7 years ago

Dante-101 commented 7 years ago

The current v6 is only distributed with ES6 syntax containing let, const, class keywords which breaks the app in old browsers with the error Uncaught SyntaxError: Use of const in strict mode.

Babel + Webpack do not transpile the distributed JS of this package in my app but do it for other packages like react-redux. I believe this is because react-redux distributes transpiled ES5 and ES6 code in different directories and refers the ES6 code using module: "es/index.js" and jsnext:main: "es/index.js" in package.json. See https://github.com/reactjs/react-redux/blob/master/package.json.

Pomax commented 7 years ago

I've pushed a v6.0.1 so that there is a "jsnext:main" entry, let me know if that appeases your build system

Andarist commented 7 years ago

@Pomax it wont fix this, the only difference between main and jsnext:main/module entries is what type of modules they might contain, latter may contain es6 import/export statements (and those will be recognized and transpiled by the modern bundler like webpack2 and rollup)

The rest of their content (other es6 features like let, const, class) wont get automatically transpiled.

As I need to ship es5 code to the browser I've setup my webpack2 with such config, so this lib gets transpiled too:

{test: /\.jsx?/, use: ['babel-loader'], exclude: /node_modules\/(?!react-onclickoutside)/},
Pomax commented 7 years ago

@Andarist it feels like it's worth putting that in the README.md as part of the installation and use section, under a "webpack + babel" subheading.

Dante-101 commented 7 years ago

@Pomax and @Andarist, Thanks for the super quick replies! :)

exclude: /node_modules\/(?!react-onclickoutside)/} works but changing webpack configuration for one package is not a desirable situation.

It will be great if sometime down the line we can release a version that doesn't need this hack and it works out of the box.

Pomax commented 7 years ago

It would be. Ideally, webpack/babel can simply autodetect that a dependency is ES6 and auto-compile it for you, and this kind of manual indicating won't be necessary anymore.

joeduncan commented 7 years ago

@Pomax what's the downside of releasing this package in es5? You can still write your package in es6 and then release both the transpiled and original version without problems. This way users of this package would not have to change their webpack config and it would work out of the box.

thebuilder commented 7 years ago

Adding the js:next/module field to package.json seems like a fine solution - But you'd still need to add the es5 compiled .js file. Right now 6.01 only contains index.js which all of the entries point to?

Pomax commented 7 years ago

@joeduncan mostly the insanity of individual libraries having to give you deoptimized code. Everything react uses a build chain already, I literally don't understand why libraries should give you ES5 code when the ES6 code lets you do dead code removal, tree shaking, etc.

Just bundle your dependencies in their ES6 form, and only as a very last step after everything's been bundled, do your ES5 conversion. Nowhere in that chain does a library need to be ES5. Browsers already support virtually all of ES6 natively (spread operator support is still a bit spotty, that's kind of it) so the idea of "you need to supply an ES5 version" strikes me as quite bizarre. This is what build tools are for: they take care of the conversions you need without needing each and every library to offer, two, three, four, etc. diffferent builds (the "dev vs. min" library is another great example of this. Minification of an entire bundle is far more efficient than bundling already individually minified libraries).

Is there a good explanation of why your build chain can't do this for you?

thebuilder commented 7 years ago

Using Babel in webpack, you normally exclude node_modules to get a massive performance boost. It would be great if could if we could all just use ES6, but for now i think it's necessary to ship both ES5 and ES6 in modules.

https://webpack.js.org/loaders/babel-loader/#babel-loader-is-slow-

test: /\.js$/,
exclude: /(node_modules)/,
joeduncan commented 7 years ago

@Pomax as far as i understand the module field should point to a version of the package with all es20XX features transpiled but still using modules - so import and export statements stay in this version. The main field should point to fully transpiled version of this package (no es20XX features and no imports/exports). The jsnext:main seems to be deprecated but its seems reasonable to leave it in for compat reasons.

https://github.com/rollup/rollup/wiki/pkg.module

In Short: here's a PR that publishes both versions: https://github.com/Pomax/react-onclickoutside/pull/194 The module version is transpiled to /es, the commonjs to /lib. I took the folder naming convention from redux since it seems to be understandable

dazlious commented 7 years ago

For everyone, who has not been able to compile it to ES5: babel in package.json overrides my configuration. So i had to do this:

module: {
        rules: [{
            test: /\.js?$/,
            exclude: /(node_modules)\/(?!react-onclickoutside)/,
            use: {
                loader: 'babel-loader',
                options: {
                    extends: `${__dirname}/.babelrc`,
                },
            },
        }],
    },

Now it is working for me!