-
-
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
Fix dayjs/esm type definitions #1581
Conversation
Just a few additional remarks about this pull request for using 'dayjs/esm' with typescript: What we want
The problems
As I didn't find a way to access types defined in a namespace via destructuring imports, I moved the types out of the namespace dayjs. This means that the types can only accessed by destructuring imports (e.g. The class 'Dayjs' is part of the 'dayjs' namespace. Plugin topicsAll types are accessible via the module 'dayjs/esm' as named exports (i.e. via a destructuring import)
|
Is it possible to re-use the type files under |
I saw that you modified the "original" type definitions in The problem arises for all type definitions that contain 'types' and 'namespaces' (e.g. Unluckily it concerns more than a few plugins: arraySupport, calendar, customParseFormat, dayOfYear, duration, isBetween, ..... And then we have plugins with more modifications necessary (e.g. I did not try to transfer the modifications back to the Sorry for that; I am absolutely no fan of code duplication, but I didn't find a better solution. |
Codecov Report
@@ Coverage Diff @@
## dev #1581 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 175 181 +6
Lines 1697 2064 +367
Branches 387 537 +150
==========================================
+ Hits 1697 2064 +367
Continue to review full report at Codecov.
|
I see. As a matter of this, we might publish another npm package Cause it has a lot different with out current umd dayjs To make it clear. |
OK, I see: 1 source (in github) 2 targets (in npm). As we need 2 package.json files anyway, that looks like a good choice to me. Unluckily there is already a |
Yes, I took it. However, it's still hard to manage two packages via the same GitHub repo source. |
For sure that is right, but perhaps some steps are or could be automated? That depends on how you do the release process. In my projects I use the package "standard-version" to generate the changelog file and modify data (e.g. the new version number) in several files in the project folder and subfolders. This way I keep everything in sync. In the case of So if this case arises and if you need some support, I would appreciate to help out - just give me a mention or so. |
Maybe, we could create a new project/repo to download the latest dayjs from the npm package |
Or, to just release [email protected] to make esm as default bundle instead of umd |
IMHO, what we have by now is:
The point 3. can easily be solved by copying the package.json from Point 4 is resolved by pr #1581, and IMHO cannot be easily solved with a script that adapts the type definitions (besides creating the right folder structure and replacing 'export =' with 'export default'). If you just release [email protected] (and kind of drop support for the umd module) you would leave all those people behind that use your lbrary today - and I think there are lots of them. And to the idea of a new project solely for the esm variant: even with a new project, you would have to modify the type definitions, when the interface of dayjs or one of the plugins is changed. |
If we release a package called For instance, a project using Then, there will be two dayjs functions that share the same global variable and may cause trouble. |
es-modules don't use global variables (that's the idea behind all those module systems: avoid collisions in global variables). Therefore this should not be the problem. |
@iamkun |
The type definition didn't allow you to use an array to define the format for parsing a date
81764cd
to
61359c1
Compare
This PR did solve some significant problems related to ESM in dayjs. However, there will be a massive breaking change if we get this PR merged. It's a hard decision to make Tbh. I'm not sure if fixing it in v2.x is a better choice? |
Of course it is; with the start of dayjs 2.0 it doesn't make sense any more to add such a hard change to the system. I am nearly finished with the issues I was commenting and the pr I created for them, so that I will concentrate to contribute a little bit to dayjs 2.0. I will close this PR - thanks for the discussion |
Make the code in the 'dayjs/esm' folder published on npmjs usable with typescript.
This pr should kind of include the modifications from pr #751 (what still remains true is the comment of @sullvn, but pr #751 does not solve this major rework of dayjs anyway).
This pr should be an answer to the problems described in issue #1281.
What my pr here does
Update the type definitions in the 'dayjs/esm' folder of the code published to npmjs, so that dayjs can be used with plain typescript and esm and without namespace imports (
import * as x
) and without any settings likeesModuleInterop
orallowSyntheticDefaultImports
. A working sample project based on this pr and with tests for all packages can be found on github.The pr contains a small modification to the 'esm.js' build script:
I used
__dirname + '../'
as replacement forprocess.env.PWD
to be consistent with the 'index.js' build script (and to make it possible to run this script on my windows machine).What remains to be done
I did not find the script to update package.json during publishing, so I have to stay with a placeholder file in 'dayjs/esm'. I suppose that in this script the version tag in package.json is updated.
Part of the solution of the problem with using dayjs as an esm package was adding a copy of the package.json to the folder 'dayjs/esm' with
type=module
andmain=index.js
in it. The rest of the content should come from the package.json in 'dayjs'. This is something that still has to be done and is not part of this pull request.