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

export = locale in index.d.ts doesnt allow synthetic import #2189

Open
Alex33400 opened this issue Jan 4, 2023 · 18 comments
Open

export = locale in index.d.ts doesnt allow synthetic import #2189

Alex33400 opened this issue Jan 4, 2023 · 18 comments

Comments

@Alex33400
Copy link

Describe the bug
When I want to import dayjs as synthetic import in my lib, I get the following error :

export = dayjs;
~~~~~~~~~~~~~~~
This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag

Expected behavior
Application can build with this line : "import dayjs from 'dayjs';"

Information

  • Day.js Version [e.g. v1.11.7]
  • OS: [Windows]
  • Browser: Chrome 108
  • Time zone: GMT +1
Alex33400 added a commit to Alex33400/dayjs that referenced this issue Jan 4, 2023
@BePo65
Copy link
Contributor

BePo65 commented Jan 8, 2023

As a starter: this does not only affect the 'core' program of dayjs (as handled in your pr #2190), but any plugin and locale file.

From your pr I suppose you are using dayjs in an esm environment. But dayjs is a commonjs library (the esm first implementation is still under construction; mostly called "dayjs 2.0", but according to the authors it will probably get 3.0).

I made a pr for fixing all the (default) type definitions for esm (pr #1581) and the answer was "let's implement it in dayjs 2.0" (at least that is my interpretation 😄).

The typescript handbook says that export = in the file ZipCodeValidator requires an import zip = require("./ZipCodeValidator"); in the module using it.
So perhaps that is an acceptable workaround until we get "dayjs 2.0".

@trim21
Copy link

trim21 commented Jan 8, 2023

From your pr I suppose you are using dayjs in an esm environment. But dayjs is a commonjs library

But it also break esm build, you can't import dayjs from 'dayjs/esm' without allowSyntheticDefaultImports flag.

Or maybe cjs and esm module should not use same export, esm/index.d.ts sould have export default dayjs instead of export = dayjs

main.ts:1:8 - error TS1259: Module '"***/node_modules/dayjs/esm/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

1 import dayjs from 'dayjs/esm';
         ~~~~~

  node_modules/dayjs/esm/index.d.ts:3:1
    3 export = dayjs;
      ~~~~~~~~~~~~~~~
    This module is declared with 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

@BePo65
Copy link
Contributor

BePo65 commented Jan 8, 2023

How about using import zip = require("dayjs") as the handbook says instead of import dayjs from 'dayjs/esm?

Or import * as dayjs from 'dayjs' as the dayjs documentation recomments?

In my (esm) application, I gave up to try to use 'dayjs/esm' because of the effects you see in your app too.

@trim21
Copy link

trim21 commented Jan 8, 2023

TBH, import * as dayjs from 'dayjs'; dayjs() is not semantic correct.

This means it import all named exports from module 'dayjs' and put them as properties of object dayjs. So it means dayjs here is a object, it's not callable, and it's also the runtime behavior, you will need to use dayjs.default() to create a Dayjs object.

But what we actually want to do is import the default export of module 'dayjs'. In fact, the document of dayjs you are referring to is wrong.

and if you are using esm, import dayjs = require("dayjs"); actually only works in .d.ts, but not .ts, you can't compile it with tsc.

main.ts:9:1 - error TS1202: Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

9 import dayjs = require("dayjs");
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error in main.ts:9

So, for an esm TypeScript developer with allowSyntheticDefaultImports flag off , there is no way to import default export of dayjs, you can't even do import { default as dayjs } from 'dayjs/esm';. the only way is to enable it, change the source of dayjs, or write a wrapper in js.

eg:
vendor/dayjs.js

import dayjs from 'dayjs/esm';

export default dayjs;

vendor/dayjs.d.ts

import dayjs = require('dayjs');

export default dayjs;

then you can import dayjs from './vendor/dayjs'

@BePo65
Copy link
Contributor

BePo65 commented Jan 8, 2023

You are right: import * as ... is for commonjs environments and not usable in your case.

It is quite a while since i wrote pr #1581, but as far as I remember the problems were:

  • the type definitions included in dayjs are for use in commonjs environments
  • during the build process a directory 'dayjs/esm' with the esm version of dayjs is generated.
  • unluckily the build script just copies the type definitions from dayjs (the commonjs version) to this directory
  • in my pr I tried to fix this by introducing a second set of type definitions that handle especially the export default problem. But the author did not like my idea of 2 sets of type definitions (I can accept this attitude) and I did not find a way to get away with 1 common type definition (AFAIK export default .. prevents the use in commonjs environments).

So perhaps a (temporary) solution could be to use a copy of the type definitions from my pr in the local "typeRoots" of your program.

@trim21
Copy link

trim21 commented Jan 8, 2023

It is quite a while since i wrote pr #1581, but as far as I remember the problems were:

I read your pr, the solution is very simple...

there is no need move any type components defs, just add a esm/index.d.ts file

/// <reference path="./locale/index.d.ts" />
import dayjs = require('dayjs')
export default dayjs;

and rename all file extention in esm to .mjs.

esm build of dayjs are doing something like import * as C from './constant';, so changeing file extension is also not a break change.

Unless someone didn't import dayjs/esm, but dayjs/esm/index.js.

And good news is import dayjs from 'dayjs/esm/index.js' doesn't work in node (esm without type: "module"), so it's not a runtime breaking change.

But this will break users write code with import dayjs from 'dayjs/esm/index.js', so just add a dummy index.js and re-export all exports of index.mjs should be fine.

https://github.com/search?q=dayjs%2Fesm%2Findex.js&type=code

@BePo65
Copy link
Contributor

BePo65 commented Jan 8, 2023

I think I still have my test project from the time of this pr. If I find it, I could try it in a commonjs environment (but not in the next 2 minutes 😄).

@trim21
Copy link

trim21 commented Jan 8, 2023

On second thought I think it could be a fix... and it doesn't touch any code with commonjs. also you can't require esm pacakge in cjs, so it only need to work with esm.

@BePo65
Copy link
Contributor

BePo65 commented Jan 8, 2023

ASAIK type definitions are for typescript and typescript can be used for esm and commonjs environments. So we need type definitions for both systems and regarding export these are different. So we need 2 different type definitins.

Or did I miss something.

@trim21
Copy link

trim21 commented Jan 8, 2023

ASAIK type definitions are for typescript and typescript can be used for esm and commonjs environments. So we need type definitions for both systems and regarding export these are different. So we need 2 different type definitins.

Or did I miss something.

because export default also works with cjs as type definition. you don't have to write export = dayjs

But that's not the point... import dayjs = require('dayjs'); export default dayjs; is a wrapper between them.

@iamkun
Copy link
Owner

iamkun commented Jan 9, 2023

Thanks for trying to figure out this historic problem.

There are some problems like #277 and #313, which do not have a proper fix in the current project design. (At least to me) Most of them are related to Typescript and ESM, which is essential at present.

Maybe a 2.0 version will be a better place to fix it and get rid of all the limitations on the current code.

@trim21
Copy link

trim21 commented Jan 9, 2023

Thanks for trying to figure out this historic problem.

There are some problems like #277 and #313, which do not have a proper fix in the current project design. (At least to me) Most of them are related to Typescript and ESM, which is essential at present.

Maybe a 2.0 version will be a better place to fix it and get rid of all the limitations on the current code.

I think #2193 at least fix something, could you take a look? it doesn't touch cjs files, and only change some code generated in dayjs/esm.

it doesn't fix the problme like import 'dayjs' but at least make import 'dayjs/esm/index.mjs' less buggy.

@BePo65
Copy link
Contributor

BePo65 commented Jan 9, 2023

As dayjs 2.0 is still a few months away, it is surely worth taking a deeper look at this lightweight extension of the dayjs esm architecture. I would appreciate this 👍

My pullrequests concerning dayjs 2.0 show that it is not so easy, to define an expandable architecture / plugin system in a typesafe environment.

@iamkun
Copy link
Owner

iamkun commented Jan 10, 2023

True, the plugin system introduced in dayjs is not really flowed the TypeScript way. That's maybe the main challenge in 2.0 version.

@trim21
Copy link

trim21 commented Jan 10, 2023

Current interface merging is already the typescript way to support plugin IMO, it's what typescript do in their document. 🤔

https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation

@BePo65
Copy link
Contributor

BePo65 commented Jan 10, 2023

That is the easy part; problems arise, when you try to add a setter to a getter defined in core via interface merging. And you have to "preview" all the expansion points needed by plugins(even plugins that you don't know of 😄).

@trim21
Copy link

trim21 commented Jan 10, 2023

when you try to add a setter to a getter defined in core via interface merging

Can I get an example?

@BePo65
Copy link
Contributor

BePo65 commented Jan 11, 2023

See pr #2128 (but it is a quite extensive pr; an implementation of the utc plugin for dayjs 2.0).

In dayjs "core" we have a "getter" named utcOffset(): number and the plugin utc should add a setter for it (utcOffset(offset: number | string, keepLocalTime?: boolean): Dayjs).

The problem is not the js code (the way dayjs 1,x does it, will of course also run in dayjs 2.0 - but not in a typesafe way), I tried several ways to solve it, using my own ideas and any "trick" I found searching stackoverflow and other resources. But I could not define the overload signature in the utc module in a way that the compiler does not show an error and that the code does not throw during runtime.

In the end I had to add the overload signature to the "core" module - but IMO a plugin should not require a modification of the module to be expanded (but it is hard to identify the "expansion" points beforehand).

Any better solutions are welcome 😄 .

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

4 participants