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

require doesn't support default exports in ES Modules in Webpack #492

Closed
iamkun opened this issue Feb 12, 2019 · 16 comments
Closed

require doesn't support default exports in ES Modules in Webpack #492

iamkun opened this issue Feb 12, 2019 · 16 comments

Comments

@iamkun
Copy link
Owner

iamkun commented Feb 12, 2019

Day.js added module in package.json in v1.8.1+ , in order to support ES Modules.

However, there's some unexpected behavior while using commonJS require with webpack

And there a discussion from webpack here, webpack/webpack#4742

const dayjs = require('dayjs') // works with 1.8.0 - 


const dayjs = require('dayjs') // broken with 1.8.1 +
const dayjs = require('dayjs').default // works with 1.8.1 +

Note: add .default is just a temporary workaround, and it's still a bug with webpack we think. Since default is an artifact of ES Modules and should not appear in CJS requires

@mweststrate
Copy link

@iamkun no, this is what was specified in your source code, here.
If you don't want a .default when using common JS, the exports should be written as module.exports = dayJS (which has no ESM equivalent). The reason why it worked before, is that because if you were exporting commonJS, babel was assuming that with require('dayjs') you were meaning require('dayjs').default (it generates some feature detecting code for that), which so far made seemed that require without default was the commonJS equivalent, but it really isn't

@iamkun
Copy link
Owner Author

iamkun commented Feb 12, 2019

@mweststrate Yes, that's why we have two bundles main: /dayjs.min.js is the umd bundle for commonjs, while module: /esm/index.js is for the ESM.

And webpack just load the module: /esm/index.js no matter it is require or import.

@mweststrate
Copy link

mweststrate commented Feb 12, 2019 via email

@githoniel
Copy link

For ESM the "namespace" object is returned by require() (and for import()). There is no spec for this, but there is a spec for import(). Babel also transpiles ESM this way.
Exposing only the default export would make all other export inaccessible and most packages un-usable from CJS.

As a guideline: When packaging CJS and ESM in a package, expose the same API from both of them. You can't use a function assigned to module.exports.

exports default "something"
export var named = "other";
exports.default = "something";
exports.named = "other"

webpack/webpack#6584
facebook/create-react-app#6321

@Tvrqvoise
Copy link
Contributor

You cannot, strictly speaking, support the old CJS interface with ESM exports since it would require making your ESM namespace the dayjs function. Why does this library support ESM at all? No tree shaking benefit, since all the functions are tacked into dayjs or manually imported from plugins.

@ghost
Copy link

ghost commented Feb 13, 2019

@Tvrqvoise I don't agree. Seems some of the imported utils and constants is tree-shakable.

@ghost
Copy link

ghost commented Feb 13, 2019

@mweststrate @githoniel And the main problem here is webpack is using the wrong file. require for commonjs should use main: /dayjs.min.js instead of module: /esm/index

@Tvrqvoise
Copy link
Contributor

@xxyx there's only one export in the library, everything else is referred to by it. Current DCE techniques can't trim that out. I've been wrong before though, if you have an example of something that you think could be tree-shook, let me know.

@iamkun
Copy link
Owner Author

iamkun commented Feb 13, 2019

@Tvrqvoise Seems we just do module: /dayjs.min.js will fix everything? I do agreed that at this time, there's no need to support ESM, but is't a nice to have feature right?

Although it is not real ESM ...

@Tvrqvoise
Copy link
Contributor

@iamkun, yes, but you'll still have the .default problem unless you also switch to CJS exports

@iamkun
Copy link
Owner Author

iamkun commented Feb 13, 2019

@Tvrqvoise

In my test, if changed like this, CJS require will be fine.

"main": "dayjs.min.js",
"module": "dayjs.min.js",

@githoniel
Copy link

@iamkun could you release a new version to resolve this?

@iamkun
Copy link
Owner Author

iamkun commented Feb 14, 2019

@githoniel

In v1.8.6, I changed module back to the umd version. This might could solve this issue, but of course not a good practice.

"main": "dayjs.min.js",
"module": "dayjs.min.js",

However, we could still use the ESM version like this

import dayjs from 'dayjs/esm'

@miladkdz
Copy link

In version 1.8.27, the type declaration file for 'dayjs/esm' is missing.

@iamkun
Copy link
Owner Author

iamkun commented Jan 4, 2021

We've add `module` entry to `esm/index` after @1.10.1, the problem in this issue may occer again.

According to webpack/webpack#4742,

"always prefer import nowadays. Only use require if the package is not an ES Module."

if you have to use require('dayjs') not import in your project (after 1.10.1), you can update your code

// 1 require the umd bundle
var dayjs = require('dayjs/dayjs.min')

// 2 require default 
var dayjs = require('dayjs').default

@iamkun
Copy link
Owner Author

iamkun commented Jan 5, 2021

After a lot of issues related to the ESM module entry.

A final decision here: #1314

This package will not ship with package.json module entry.

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

No branches or pull requests

5 participants