Skip to content
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

Module Inclusion #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Module Inclusion #40

wants to merge 1 commit into from

Conversation

vdegenne
Copy link

@vdegenne vdegenne commented Dec 30, 2019

Hello,

I tried to use your package in an es module context only to realize it didn't work because your main index.js file possesses commonjs syntax.
I decided to fork and modify the code to include module and umd support using microbundle (more info: https://github.com/developit/microbundle).

I didn't add or remove procedures, I just transformed the index.js into ES6 Module syntax which is now supported by modern browsers. There's a build step which converts this file into different targets (into dist) :

  • CommonJs (fallback) for people using plain JS in Node.JS scripts.
  • ESM (along with the module property in package.json) for people using ES6 Modules for modern browsers, experimental Node.JS module support (future), and TypeScript module resolution.
  • UMD for Node & CDN's

The test passes and the module.exports is unchanged which means existing projects won't be affected. I also tested the compilation result in my initial project and the node resolver uses the mjs as intended (i.e. @vdegenne/clipboard-copy for live testing).

I think this is one step into JavaScript evolution towards trying to make modular programming easier. And this PR should be considered.

If you merge this PR, the next step for you is to build the distribs npm run build, change the versioning, and push the update to the npm repository for everyone's sake. Also modifying the readme according to the new terms.

Have a nice day.

@feross
Copy link
Owner

feross commented Nov 18, 2020

Thank you for this PR. I would like to support ESM in this package, but I don't like how microbundle seems to do a lot more than just this. It's doing some transpiling of async-await to Promise, etc.

Is there are an tool that just takes ESM and outputs CJS and updates package.json? cc @mikeal

@TrySound
Copy link

Rollup would work fine in this case. Microbundle is based on it.

@TrySound
Copy link

Btw would you like to add node es modules support as well?

@LinusU
Copy link
Collaborator

LinusU commented Nov 19, 2020

Hmm, since there is no need for this package in Node.js (right?), couldn't we just switch to just doing ES modules and skip the build step altogether? and release that as a new major version...

It seems that that is much easier for us to maintain, and would still be supported by all bundlers, and modern web browsers without a bundler.

e.g. after this change, when working on the package, I know have to remember to run npm run watch or otherwise I will test old code. And I also run the chance of reloading to quickly in the browser after making a change, and then testing the previous version without my latest changes...

@feross
Copy link
Owner

feross commented Nov 23, 2020

and would still be supported by all bundlers

Are ESM-only packages supported by browserify?

@benmccann
Copy link

Are ESM-only packages supported by browserify?

Would you even need to use Browserify at that point? ESM packages are natively supported in the browser. I have seen some posts suggesting you might be able to Babelify with Browserify to use ESM-only packages, but have not tried it.

@benmccann
Copy link

You could also support ESM by adding a wrapper which wouldn't require using Rollup or Microbundle: https://nodejs.org/api/packages.html#packages_approach_1_use_an_es_module_wrapper

If we can go ESM-only that'd still be preferable for reduced complexity, but if we need to support both it wouldn't be too hard. Let me know what path forward you'd prefer and I can help

@benmccann benmccann mentioned this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants