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

Fix dayjs/esm type definitions #1581

Closed
wants to merge 3 commits into from

Conversation

BePo65
Copy link
Contributor

@BePo65 BePo65 commented Jul 17, 2021

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 like esModuleInterop or allowSyntheticDefaultImports. 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 for process.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 and main=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.

@BePo65
Copy link
Contributor Author

BePo65 commented Jul 17, 2021

Just a few additional remarks about this pull request for using 'dayjs/esm' with typescript:

What we want

  1. a default export to access the factory functions, the methods and the types
  2. named exports for the methods and the types to be used in destructoring imports

The problems

  • for (1) we use overloading: we need 3 elements (the name doesn't matter, but 'dayjs' is used here)
    • 2 overloaded versions of the factory function and
    • a namespace with the methods and types of dayjs.
  • for (2) we need named exports of the methods and types

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. import { <name-of-type> } from 'dayjs/esm'), not via the default import (import dayjs from 'dayjs.esm').

The class 'Dayjs' is part of the 'dayjs' namespace.

Plugin topics

All types are accessible via the module 'dayjs/esm' as named exports (i.e. via a destructuring import)

  • duration (DurationAddType, DurationInputType);
    • according to issue Breaking changes in 1.10.4  #1360 to use the (deprecated) types we have to use destructuring imports;
    • the default import does not return the types for destructuring
    • if we wrap the type definitions in a namespace (required to acccess the types via the default import) , we cannot use destructuring imports, only the default import.
    • ==> I added the types to the 'dayjs/esm' module as named export (I did that for all types to be consistent with the dayjs type definitions).
  • localeData: types go to the module 'dayjs/esm'; interfaces and methods go to the namespace 'dayjs' (to make them accessible via the default import)

@iamkun
Copy link
Owner

iamkun commented Jul 19, 2021

Is it possible to re-use the type files under /type instead of copying them?

@BePo65
Copy link
Contributor Author

BePo65 commented Jul 19, 2021

I saw that you modified the "original" type definitions in esm.js. Basically a great idea. For some plugins that would still be possible.

The problem arises for all type definitions that contain 'types' and 'namespaces' (e.g. index.d.ts ind the base folder or duration.d.ts in the plugin folder). Here the necessary modifications are so extensive that IMHO it cannot be done by code.

Unluckily it concerns more than a few plugins: arraySupport, calendar, customParseFormat, dayOfYear, duration, isBetween, .....
In that definition files, after any declare module 'dayjs' { we would have to insert namespace dayjs {.

And then we have plugins with more modifications necessary (e.g. duration with its deprecated type defnitions).

I did not try to transfer the modifications back to the dayjs, but I am afraid that would break most applications.

Sorry for that; I am absolutely no fan of code duplication, but I didn't find a better solution.

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #1581 (61359c1) into dev (9ef1714) will not change coverage.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
src/locale/br.js 100.00% <ø> (ø)
src/locale/de.js 100.00% <ø> (ø)
src/locale/es-do.js 100.00% <ø> (ø)
src/locale/es-pr.js 100.00% <ø> (ø)
src/locale/es-us.js 100.00% <ø> (ø)
src/locale/es.js 100.00% <ø> (ø)
src/locale/fi.js 100.00% <ø> (ø)
src/locale/lt.js 100.00% <ø> (ø)
src/locale/nb.js 100.00% <ø> (ø)
src/locale/pl.js 100.00% <ø> (ø)
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a43708...61359c1. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Jul 30, 2021

I see.

As a matter of this, we might publish another npm package dayjs-esm or something alike?

Cause it has a lot different with out current umd dayjs

To make it clear.

@BePo65
Copy link
Contributor Author

BePo65 commented Jul 30, 2021

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 dayjs-esm on npm (without any information about author. source etc.). I hope it's your package :-)

@iamkun
Copy link
Owner

iamkun commented Aug 2, 2021

Yes, I took it.

However, it's still hard to manage two packages via the same GitHub repo source.

@BePo65
Copy link
Contributor Author

BePo65 commented Aug 3, 2021

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 dayjs what really remains 'troublesome' and where I don't know how to script in a robust way, is the generation of the 2 sets of type definitions. But here modifications are only necessary, if the interface changes - i.e. not too often.

So if this case arises and if you need some support, I would appreciate to help out - just give me a mention or so.

@iamkun
Copy link
Owner

iamkun commented Aug 3, 2021

Maybe, we could create a new project/repo to download the latest dayjs from the npm package dayjs@latest after each dayjs release, and update the folder structure as well as the typefiles in esm way. And publish it as dayjs-esm

@iamkun
Copy link
Owner

iamkun commented Aug 3, 2021

Or, to just release [email protected] to make esm as default bundle instead of umd

#1281

@BePo65
Copy link
Contributor Author

BePo65 commented Aug 3, 2021

IMHO, what we have by now is:

  1. a library in path dayjs that can be used with javascript as an umd module with require or import
  2. matching type definitions for the umd module to use dayjs in a typescript project using import dayjs from 'dayjs'
  3. a library in path dayjs/esm that throws an error when used in a javascript esm project (i.e. using "type": "module"), as there is no package.json in 'dayjs/esm' that also has a "type": "module" setting
  4. type definitions for the esm module in dayjs/esm that are modified with a npm script to use export default, but throw when using the plugins.

The point 3. can easily be solved by copying the package.json from dayjs to this subfolder and adding a "type": "module" line.
For example this could be done when building the esm folder.
When the version number in the main package.json file is updated, then also the version number in this package.json in dayjs/esm must be set.

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.
So I would prefer to keep everything in one place (except if you want to switch the basic structure to classes instead of the prototype modifications; but this would be a major refactoring).

@iamkun
Copy link
Owner

iamkun commented Aug 4, 2021

If we release a package called dayjs-esm in es-module, this will come to another problem.

For instance, a project using dayjs-esm as a dependency, while it's another dependency (maybe a Calendar component), depends on dayjs.

Then, there will be two dayjs functions that share the same global variable and may cause trouble.

@BePo65
Copy link
Contributor Author

BePo65 commented Sep 12, 2021

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.

@hakimio
Copy link

hakimio commented Oct 15, 2021

@iamkun dayjs is one of the last widely used libraries which doesn't have proper ES module support. It would be really nice if you could decide if you want fully migrate to ESM and drop support for CJS (which imho is the best option) or go with the solution in this PR and have some duplicated code.

@BePo65 BePo65 force-pushed the fix/esm-type-definitions branch from 81764cd to 61359c1 Compare May 4, 2022 04:36
@iamkun
Copy link
Owner

iamkun commented Jun 11, 2022

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?

@BePo65
Copy link
Contributor Author

BePo65 commented Jun 11, 2022

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 ☺️

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

Successfully merging this pull request may close these issues.

3 participants