Proposal to use Rollup to build as ESM and CJS

[abdonrd]

Fix #8.

Based on #9.

[coveralls]

Coverage remained the same at 100.0% when pulling 2048949879e84bf8af997d83745a79881adc5199 on abdonrd:rollup-build into b3ab8bdfb91cb182c93475c2c3518d6224672bb4 on epoberezkin:master.

[tkrns]

Please consider merging this pull request.

[Tansito]

Very interested in this PR too!

[ruanxianzhi]

Will this will be merged soon?

[epoberezkin]

9 is merged.

I don't think that using "require" is the problem that needs to be solved at the cost of introducing build step into the library that has only one file and no dependencies.

A much simpler solution would be to have a one-line wrapper that uses require and exports as ESM (that can be either in the application or even be in this package with .mjs extension).

[abdonrd]

Rebased!

---

Thanks for the answer, @epoberezkin!

The thing is that you can't use require / module.exports (CJS) directly in the browser. But you can use the standard import / export (ESM).

[abdonrd]

An example working directly in the browser, without any build step: https://glitch.com/edit/#!/rift-coneflower

Note that the package.json has:

"fast-json-stable-stringify": "abdonrd/fast-json-stable-stringify#es-modules"

And the JS code is just:

import stringify from 'fast-json-stable-stringify';

var obj = { c: 8, b: [{z:6,y:5,x:4},7], a: 3 };
document.getElementById('result').innerText = stringify(obj);

[abdonrd]

The same example, but with the last published version (as CJS): https://glitch.com/edit/#!/atlantic-hardboard

Error in the console:

Uncaught SyntaxError: The requested module './node_modules/fast-json-stable-stringify/index.js' does not provide an export named 'default'

[marshallswain]

@abdonrd it seems to me that the appropriate solution to this would be to create the separate wrapper file as @epoberezkin has requested. The build step is for sure required for browser compatibility, but if I comprehend what @epoberezkin has requested, he doesn't see a reason to add a build step for the primary export. My suggestion is to

• [ ] Restore the index.js to its previous, CommonJS state.
• [ ] Create the separate wrapper file next to the current index.js
• [ ] Target the build system to the wrapper and export to ESM format. I'd recommend something like /browser.js or /esm.js.

@epoberezkin if the above are accomplished, would it be ok to merge this? I'm ending up with support requests for problems this is causing for Feathers-Vuex users. I personally can't replicate the issue in my apps, so I don't know what's causing the issue for them.

If you would prefer to leave your package as is, I could copy the source into Feathers-Vuex and transpile it internally. That would allow you to leave your package as is.

[abdonrd]

As long as the JavaScript import standard is used (ECMAScript modules), the browser does not need any build step. In fact, now even Node.js 13.2.0 (November 2019) shipped support for ECMAScript modules.

Using the standard in the source code seems right to me. And use a build step for backwards compatible.

[marshallswain]

That makes sense to me. I wasn’t aware of Node 13 ESM support.

[epoberezkin]

So in this case do we just need the wrapper that would work in 13.2+? Or do we still need to process it? I will review once again.

[abdonrd]

Mi idea is:

1. Update the source code to ESM:

// index.js

export default function (data, opts) {
  // ...
};

And then, this index.js will work in any modern browser and in NodeJS ^v13.2 out the box. You can see a live example here: https://github.com/epoberezkin/fast-json-stable-stringify/pull/10#issuecomment-565810328

2. With Rollup, get the source code and export a CJS version to backwards compatible.

// index.cjs.js

module.exports = function (data, opts) {
  // ...
};

3. Update the package.json as:

  "main": "index.cjs.js",
  "module": "index.js",

Done! 🎉

[marshallswain]

By the way @epoberezkin, thank you for this great project. It definitely lives up to its name.

[tirithen]

This pull request is much wanted, we still use my fork for production, it would be so nice to remove that and use this repos original package instead. Is there any missing pieces left?

[abdonrd]

Thanks @tirithen! From my side, just waiting for @epoberezkin.

[abdonrd]

Closing because no response.

[thescientist13]

Hoping the team can confirm at least if there is interest in accepting / merging this work (and if there is anything needed to help get it there)?

Understand if time may be a factor, but it would be good to know at least that native ESM support has a future in this project. 🙌

Thanks!