-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal to use Rollup to build as ESM and CJS #10
Conversation
Please consider merging this pull request. |
58f2294
to
ca9d811
Compare
Very interested in this PR too! |
Will this will be merged soon? |
#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). |
08c8c00
to
8af13f1
Compare
Rebased! Thanks for the answer, @epoberezkin! The thing is that you can't use |
An example working directly in the browser, without any build step: Note that the
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); |
The same example, but with the last published version (as CJS): Error in the console:
|
@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
@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. |
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. |
That makes sense to me. I wasn’t aware of Node 13 ESM support. |
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. |
Mi idea is:
// index.js
export default function (data, opts) {
// ...
}; And then, this
// index.cjs.js
module.exports = function (data, opts) {
// ...
};
"main": "index.cjs.js",
"module": "index.js", Done! 🎉 |
By the way @epoberezkin, thank you for this great project. It definitely lives up to its name. |
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? |
Thanks @tirithen! From my side, just waiting for @epoberezkin. |
Closing because no response. |
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! |
Fix #8.
Based on #9.