MaxArt2501 / share-this

Medium-like text selection sharing without dependencies
https://maxart2501.github.io/share-this/
MIT License
808 stars 53 forks source link

The ES module points to non-transpiled src/core.js file #48

Open markogresak opened 4 years ago

markogresak commented 4 years ago

This module claims support for various older versions of browsers.

But there is an edge case where this is not true.

When used with webpack or similar tools, it's a common practice to use import. Even the docs suggest using

// ES6
import shareThis from "share-this";

But there's a catch. The module resolver prefers module over main in package.json. This is true only for JS Modules (import syntax).

https://github.com/MaxArt2501/share-this/blob/v1.3.1/package.json#L13-L14

  "main": "dist/share-this.js",
  "module": "src/core.js",

In the case of share-this, it means that the import syntax will take the src/core.js. The usual webpack config will not compile anything from node_modules folder for performance reasons.

So when we put all of this together, the result is the raw src/* code landing in the vendor bundle, with const and everything. Internet Explorer says πŸ‘Ž πŸ˜„


There are a few possible solutions:

  1. Ship a transpiled dist/share-this.es.js and change the module to point to it.
  2. Remove the module and leave only the main. Most current build pipelines will work with the old commonjs way. Not sure if this will hold true in the future.
  3. Leave it to the consumers to figure it out, either import share-this/dist/share-this or transpile it on their end. It's like this at the moment, so no work required. It's a solution, but a bad one.
MaxArt2501 commented 4 years ago

I claim no responsibility for what webpack does :| But you can configure it to compile certain modules in node_modules. Granted that last time I wrote that part in the package.json for this library it wasn't clear at all what the "module" property would have implied, as it is you can do this:

import shareThis from 'https://cdn.jsdelivr.net/npm/share-this/src/core.js';

and that would work perfectly fine.

But I guess I'll look into what's rolling now for those package.json properties, and serve a single file compiled for ES6 in "module"...

markogresak commented 4 years ago

I claim no responsibility for what webpack does

To clarify, it's not just webpack, any "standard" bundler implementation where we use import ... from ... will behave this way. It's even mentioned in the Node.js docs https://nodejs.org/api/esm.html#esm_dual_commonjs_es_module_packages

Prior to the introduction of support for ES modules in Node.js, it was a common pattern for package authors to include both CommonJS and ES module JavaScript sources in their package, with package.json "main" specifying the CommonJS entry point and package.json "module" specifying the ES module entry point. This enabled Node.js to run the CommonJS entry point while build tools such as bundlers used the ES module entry point, since Node.js ignored (and still ignores) the top-level "module" field.

So from this point of view, the use of "module" is not correct, it should be "exports". But it's the de-facto way with bundlers.

Also, the bug I have reported here is the problem node.js docs call "dual package hazard": https://nodejs.org/api/esm.html#esm_dual_package_hazard.

So what I've suggested with option 2), removing the "module" bit sounds like the best option. Or it could be changed to "exports".