GoogleChromeLabs / react-adaptive-hooks

Deliver experiences best suited to a user's device and network constraints
Apache License 2.0
5.1k stars 113 forks source link

const, let and spread (not IE11 compatible) #6

Closed theKashey closed 4 years ago

theKashey commented 4 years ago

This library is expected to be consumed as is, however, without implicit include for babel-loader it will emit the original ES6 code to the final bundle, breaking old browsers.

Proposal:

PS: And is there any reason so spread result like return { ...memoryStatus };

OriginalEXE commented 4 years ago

This library is expected to be consumed as is

Not sure what you mean? I don't think libraries today should be written in ES5 just for the compatibility sakes. People who need older browsers compatibility are hopefully using some build process where babel can be already introduced. Given that we are talking about a React library, the chances of a build process not existing are (I assume) pretty small.

wmertens commented 4 years ago

@OriginalEXE prevailing opinion so far is to not transpile node_modules, instead relying on them being ES5.

However, for IE11 compatibility's sake I've had to resort to transpiling all browser-destined code, although it should definitely be avoided for server-side code, since those modules often fail to transpile (due to native code or huge size). It's not easy to set up.

OriginalEXE commented 4 years ago

Doesn't that hurt the people who do not need to support older browsers, by making them ship more code unnecessarily?

Perhaps then the best solution is indeed some kind of multi-bundle shipping.

wmertens commented 4 years ago

@OriginalEXE personally, I make 2 builds: one for browsers that understand modules and one for older. https://jakearchibald.com/2017/es-modules-in-browsers/

addyosmani commented 4 years ago

I had wanted to circle back on whether we needed to go for multi-build depending on community feedback. I would be happy to review a PR introducing two builds if someone would like to try out an approach.

developit commented 4 years ago

In general, the compromise the community has arrived at is to publish both ES2017 and ES5 builds. Within the next few months, bundlers like Rollup and Webpack will begin to support for Node's new "package exports" package.json annotations, which brings a community-standardized mechanism for shipping not just CJS and ESM, but ES-current and ES5:

{
  "main": "dist/es5-cjs.js",
  "module": "dist/es5-esm.js",
  "exports": {
    ".": {
      "default": "src/index.js"
    }
  }
}

In the meantime, if the source here is strictly ES2017 (no JSX, Babel extensions, class properties, etc), a small number of bundlers are configured to look for a "jsnext:main" package.json property, and will interpret this as a signal that the package contains ES2017 code, triggering transpilation when bundling for older browsers (and using the source as-is for modern browsers).

For now, I'd recommend shipping ESM and CJS using the "module" and "main" fields, unless the package is dramatically smaller when left as ES2017. Generally I'd recommend trying a solution like microbundle, which will generate these outputs automatically, and also now includes support for compiling to modern JS.

Andarist commented 4 years ago

small number of bundlers are configured to look for a "jsnext:main" package.json property, and will interpret this as a signal that the package contains ES2017 code, triggering transpilation when bundling for older browsers (and using the source as-is for modern browsers).

Out of curiosity - which bundlers do that? Wasn't jsnext:main an early version of module field? I've never encountered it being treated any different and from what I remember it got also deprecated in rollup-plugin-node-resolve (it ain't handled by default).

addyosmani commented 4 years ago

For now, I'd recommend shipping ESM and CJS using the "module" and "main" fields, unless the package is dramatically smaller when left as ES2017. Generally I'd recommend trying a solution like microbundle, which will generate these outputs automatically, and also now includes support for compiling to modern JS.

Thank you, @developit. microbundle has served us well in a few other projects where we've considered shipping both ESM and CJS. As we haven't really run into friction with it, using it for this package seems worthwhile to try.