-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add cjs support as conditional exports #183
base: master
Are you sure you want to change the base?
Conversation
@pravi This approach will double the package size right? |
@chase-manning yes, but it will allow people still using commonjs to continue using dateformat. Better than being stuck at old version. |
Also people using bundlers like webpack won't see a size increase in final bundle. |
This would be really helpful. Currently we are stuck with using an old version because there are still tools that don't play nice with modules (I'm looking at you, jest and typescript). The support is getting better every month, so hopefully cjs support can be removed after some time, but I'd argue we are not there yet. The ~9k of increased package size is real, but I would still consider it a good trade-off. Thank you! |
This patch doesn't produce the wanted behavior: |
As @guimard mentioned, this will need a change in API, for example, grunt needs this patch
Any ideas to get the old behavious using just babel? @babel/plugin-proposal-export-default-from plugin and babel-plugin-add-module-exports don't change anything. |
This was fixed in debian by https://salsa.debian.org/js-team/node-dateformat/-/commit/83b246a24e56b5f87777d3fc7a9676b7886d1299 any better ideas? |
@pravi Could we please make some changes to this PR? Can we please update the Readme to include reference to both import options. Also, can we please add tests for importing and using the package using the cjs version. |
@chase-manning please check if the changes are fine. I have converted one test to CJS, if you think this approach is fine. I can copy the remaining tests too. Or we can add a test_helper.esm and test_helper.cjs files which will handle the ESM and CJS parts and may be have some logic to detect cjs or esm in the tests. But I'm not sure how to do that. |
Where is it five monthes later ? |
Any plan to merge? |
Error [2022-10-23T18:48:13.701Z] Stack: Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/xxxxx/node_modules/dateformat/lib/dateformat.js |
any news update? |
Any updates on this? |
@chase-manning |
Any updates? |
This adds support for both cjs and esm versions (Closes #176)