-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
base: master
Are you sure you want to change the base?
Module Inclusion #40
Conversation
Thank you for this PR. I would like to support ESM in this package, but I don't like how Is there are an tool that just takes ESM and outputs CJS and updates |
Rollup would work fine in this case. Microbundle is based on it. |
Btw would you like to add node es modules support as well? |
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 |
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. |
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 |
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 (intodist
) :module
property inpackage.json
) for people using ES6 Modules for modern browsers, experimental Node.JS module support (future), and TypeScript module resolution.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 themjs
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.