-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
dayjs
does not actually support Node.js ESM
#1765
Comments
Any news here? |
2.0 supports ESM |
import dayjs from 'https://cdn.jsdelivr.net/npm/dayjs@2.0.0-alpha.2/dist/index.mjs'
console.log(dayjs()) // It works 👍🏼 |
Looks like the 2.0 alpha may be abandoned -- no new commits or releases in 6 months. Is there a plan to re-open work on 2.0, or to bring ESM support to 1.0? |
BUMP |
Any ETA on ESM arrival on NPM? |
I'm also waiting for Node.js ESM support |
Would be great to get ESM support! |
Another vote here for ESM support. |
Come on! Where is ESM support? |
## Summary Adding `dayjs` to `opensource`, as it's needed by the translation framework. ## Test plan Place `experimental.PagePresence` on the testbed, hover on avatar of users who haven't been present in a long time, check the tooltip has the right subtitle ## Old summary, before Josh moved us to ESM This is the second attempt at this. The first one was reverted (#7592) because it crashed at runtime. The original PR (#7473) had some `@ts-ignore` and was using `require`, which is why the build succeeded even though it should have not. `import * as dayjs from 'dayjs';` is a valid import according to `tsc`. But then `dayjs(date)` errors ``` Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead ``` It also **crashes at runtime**. Changing it to `dayjs.default(date)` doesn't crash at runtime, but `tsc` now complains about it ``` Property 'default' does not exist on type 'typeof import("/Users/albert/monorepo/opensource/sdk-js/node_modules/dayjs/index.d.ts")' ``` `import dayjs from 'dayjs';` is **not** a valid import according to `tsc` ``` This module is declared with 'export =', and can only be used with a default import when using the 'esModuleInterop' flag. ``` But **actually works** (i.e. no runtime crash). Peeking into `dayjs` codebase, I see an [export default dayjs](https://github.com/iamkun/dayjs/blob/f0c9a41c6ec91528f3790e442b0c5dff15a4e640/src/index.js#L467C21-L467C21), which is ESM syntax, which I would expect to just be able to import (`import dayjs from 'dayjs';`). My IDE is not complaining, the code is not crashing at runtime, but `npm run tsc-once` is yelling at me. [There is a long-standing issue in DayJS regarding Node.js ESM](iamkun/dayjs#1765)) which might be related. But I'm still confused 😬 Test Plan: Use the experimental.PresenceFacepile in the testbed, hover on an avatar, check the tooltip's subtitle is correct. Reviewers: Netceer, lazybean Reviewed By: lazybean Pull Request: getcord/monorepo#7750 monorepo-commit: 9ea55f11df0954eff1e344bcf12c1f9c2a474541
Damn I just wasted hours on this 🤦♂️ When converting a large codebase to a monorepo, I kept getting errors like this at runtime:
I couldn't figure it out because my bundled shared packages output looked correct, but I assumed it must be a mistake in some configuration. I tried everything I could think of before landing here. I did not imagine a popular library like this would not have support for ESM yet in 2024. I'll give the alpha 2.0 a try. Also found this issue describing that it should work by adding the .js import. It is unexpected, because extension prefixes are only required on ESM imports from relative paths, and never from an external library, but that must be a side effect of the funky not-really-esm exports. |
The types in 2.0 alpha didn't seem to work for me. For example, TS didn't recognize that |
Running SSR build with Svelte/Vite
Not sure if this is the fix you want, but I simply updated all the JS references to be |
Another vote here for ESM support. |
Pretty much, I'm just waiting patiently for https://github.com/tc39/proposal-temporal to hit Stage 4 so I can migrate my Day.js code to that 🙏 |
The current version covers my use case, time to fork. |
@codyfrisch: With all of the folks forcing us to ESM, regardless of technical benefit, this seems like it's going to continue to be a problem. |
You're coming in hot, friend. I'm talking about the trend of module maintainers changing away from supporting commonjs to ESM-only, and suggesting that it's "where the ecosystem is going" while only considering the preferences TypeScript enthusiasts and React developers. There are a number of folks using Node for automation and systems-level scripts, and it seems all of the front-end web enthusiasm is the only thing anybody really cares to consider when making these decisions. |
I'm sorry friend, but you being fine with something is not an adequate substitute for the broader community being considered in decision-making. Folks making decisions to break away from one approach or another do have an impact on a great number of people, and as a maintainer of several projects myself I'm always careful to be cognizant of that fact. I am genuinely glad to hear that this time it wasn't you. As you continue your own work, I sincerely hope that you continue your streak of good luck. How we treat each other does matter. |
Node.js has implemented native support for ESM in the real world, and the community is increasingly recommending the use of ESM instead of CommonJS (e.g: https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77).
I noticed that
dayjs
actually publishes theesm
directory, which contains the compiled product of the ESM syntax, see #1298. Although the"module"
field ofpackage.json
was removed in #1314, the contents ofesm
are actually still available in the<script type="module">
in browsers or in bundling tools such aswebpack
.Unfortunately, Node.js requires more than that for ESM. Since this is a CommonJS package, the ESM files it provides must use the
.mjs
extension to be properly parsed by Node.js.I can get a few ways to solve this problem, but perhaps none of them are very good:
.js
extension with.mjs
inbuild/esm.js
, but obviously this will cause breaking changes for scenarios that already use ESM files.node-esm
directory for.mjs
files for Node.js. Butnode-esm
is not a good name for forward compatibility reasons.dayjs-esm
package, just likelodash-es
..mjs
extension. Just keep using.js
.I'm not sure if you have plans for this in terms of Node.js ESM, but I think that this is the way to go.
The text was updated successfully, but these errors were encountered: