dmtrKovalenko / date-io

Abstraction over common javascript date management libraries
MIT License
715 stars 92 forks source link

Type augmentations included in published package fail downstream compilation #52

Closed rosskevin closed 5 years ago

rosskevin commented 5 years ago

From https://github.com/mui-org/material-ui-pickers/issues/1074#issuecomment-504511836

I'll work on a PR, I'm going to try a couple of things, but worst case is we need to not publish the type dir in the package as it does not satisfy the compiler.

dmtrKovalenko commented 5 years ago

umm, that's not an idea Because this type is unique for each lib. The idea was "magic" declaration on input. And it worked pretty well. But I think only with babel :( This doesn't work with typescript compiler?

rosskevin commented 5 years ago

Yes I am seeing the import from index now, so I see what you were up to. No it does not work with tsc. By the way, babel provides zero type checking - it only strips typescript information - so it is not a test of the types.

I'm experimenting locally to see if I can get something to satisfy tsc.

rosskevin commented 5 years ago

BTW - the only solution for type checking right now is to rm node_modules/@date-io/date-fns/type/index.d.ts - which is worse than a manual instruction to add the augmentation to the user.

I think we might be able to get this to work, I'll get back to you shortly.

dmtrKovalenko commented 5 years ago

I don't understand. Everything works for me

rosskevin commented 5 years ago

I'll track my steps here. Out of the box using:

"@material-ui/pickers": "^3.1.1",
"date-fns": "^2.0.0-alpha.27",
"@date-io/date-fns": "^1.3.7",
"typescript": "^3.5.2",

tsc fails:

> Executing task: /Users/kross/projects/js/node_modules/.bin/tsc -b /Users/kross/projects/js/tsconfig.json <

node_modules/@material-ui/pickers/typings/date.d.ts:1:26 - error TS2307: Cannot find module '@date-io/type'.

1 import { DateType } from '@date-io/type';
                           ~~~~~~~~~~~~~~~

node_modules/@material-ui/pickers/views/Year/YearView.d.ts:2:26 - error TS2307: Cannot find module '@date-io/type'.

2 import { DateType } from '@date-io/type';

I'm fairly sure it is because tsc is not going to allow a module augmentation coming from the @date-io/date-fns/index.d.ts. I believe it is invalid for anything there to include something like declare module "@date-io/type" - it will scope/resolve everything here as part of the parent @date-io/date-fns, not add the @date-io/type to the global namespace.

rosskevin commented 5 years ago

I can see that the pickers runs because you include https://github.com/mui-org/material-ui-pickers/blob/next/lib/typings.d.ts as an augmentation in https://github.com/mui-org/material-ui-pickers/blob/next/lib/tsconfig.json#L19

So this may make sense for that project (since it is agnostic of the adapter) but it is not testing the types that are attempting to be introduced as augmentations here.

I'm still looking.

dmtrKovalenko commented 5 years ago

It worked for my own project

rosskevin commented 5 years ago

Unfortunately when I include an augmentation in my app (just like pickers does), it complains of duplication:

Executing task: /Users/kross/projects/js/node_modules/.bin/tsc -b /Users/kross/projects/js/tsconfig.json <

node_modules/@date-io/date-fns/type/index.d.ts:2:15 - error TS2300: Duplicate identifier 'DateType'.

2   export type DateType = Date;
                ~~~~~~~~

  typings/my-date-io.d.ts:2:15
    2   export type DateType = Date
                    ~~~~~~~~
    'DateType' was also declared here.

typings/my-date-io.d.ts:2:15 - error TS2300: Duplicate identifier 'DateType'.

2   export type DateType = Date
                ~~~~~~~~

  node_modules/@date-io/date-fns/type/index.d.ts:2:15
    2   export type DateType = Date;
                    ~~~~~~~~
    'DateType' was also declared here.
rosskevin commented 5 years ago

I can only surmise that it works for you due to something like an order of import coincidence.

As-is:

  1. it does not automatically work for me through dependencies
  2. I cannot augment it to work (like pickers does)

I don't think we can rely on order of imports (that's speculation) or something like that to work.

rosskevin commented 5 years ago

Just for context: this error is happening in our internal yarn workspaces monorepo of reused libraries. It is not an app with an index, so I cannot control order of imports.

rosskevin commented 5 years ago

So far I have been unable to get anything to work. My only workaround is to to replicate the @material-ui/pickers setup by:

I'm not sure there is any other way. I'm open to any ideas to remove my workaround and get this to work the way it is intended.

dmtrKovalenko commented 5 years ago

Closing that. Here is a repo where auto-linked @date-io/type module works like a charm

ideoclickVanessa commented 4 years ago

I'm trying to use the pickers with the @date-io/luxon package and tsc is failing:

node_modules/@material-ui/pickers/typings/date.d.ts:1:26 - error TS2307: Cannot find module '@date-io/type'.

1 import { DateType } from '@date-io/type';
                           ~~~~~~~~~~~~~~~

node_modules/@material-ui/pickers/views/Year/YearView.d.ts:2:26 - error TS2307: Cannot find module '@date-io/type'.

2 import { DateType } from '@date-io/type';

I'm not allowed to commit to our code base with tsc errors present. Please fix the typing issue.

artisanbhu commented 4 years ago

Any alternative solution then that as @rosskevin mentioned above,

So far I have been unable to get anything to work. My only workaround is to to replicate the @material-ui/pickers setup by:

removing the published type "postinstall": "rm -f node_modules/@date-io/date-fns/type/index.d.ts" add my own augmentation my-date-io.d.ts and include in tsconfig declare module '@date-io/type' { export type DateType = Date } I'm not sure there is any other way. I'm open to any ideas to remove my workaround and get this to work the way it is intended.

artisanbhu commented 4 years ago

Any alternative solution then this, https://github.com/dmtrKovalenko/date-io/issues/52#issuecomment-631844736

dmtrKovalenko commented 4 years ago

Why removing date-io module declaration? It should work without it.

And overall this feature works perfectly without any workarounds. Could you provide a reproduction where it doesn’t work.

artisanbhu commented 4 years ago

Basically the problem is same, We have a package where we are using @material-ui/pickers, with @date-io/moment. This package builds fine because, we have import MomentUtils from '@date-io/moment. `/* @jsx jsx / import { jsx } from '@emotion/core'; import React from 'react'; import { IDatePicker } from './interfaces'; import { CustomDatePicker, DatePickerContainer } from './DatePicker.style';

import moment, { Moment } from 'moment'; import MomentUtils from '@date-io/moment'; import { DatePicker as DateSelector, MuiPickersUtilsProvider } from '@material-ui/pickers';

export const DatePicker = (props: IDatePicker): React.ReactElement => { const [isOpen, setIsOpen] = React.useState(false); return (

setIsOpen(true)} onClose={(): void => setIsOpen(false)} value={props.selectedDate} onChange={(date: Moment): void => props.handleDateChange(moment(date).toDate())} />
);

}; If you look at the peer dependency section, .... }, "peerDependencies": { "moment": "2.x", "react": "16.x", "react-dom": "16.x", "@date-io/moment": "1.3.x", "@material-ui/pickers": "3.2.x" }, ...`

When we consume this package in another package, i am getting typescript error like this,

ttsc -p tsconfig.json --noEmit

node_modules/@material-ui/pickers/typings/date.d.ts:1:26 - error TS2307: Cannot find module '@date-io/type'.

1 import { DateType } from '@date-io/type';



node_modules/@material-ui/pickers/views/Year/YearView.d.ts:2:26 - error TS2307: Cannot find module '@date-io/type'.

2 import { DateType } from '@date-io/type';

In order to get around this problem, i had to follow suggested comment, https://github.com/dmtrKovalenko/date-io/issues/52#issuecomment-631844736 . This is ugly because we had to change the tsconfig to all the consumer packages.

`   .....   
                "noUnusedLocals": true,
        "resolveJsonModule": true
    },
    "include": ["src",  "node_modules/@date-io/moment/type/index.d.ts"]
}`
whitphx commented 2 years ago

Hi @dmtrKovalenko

I think this error occurs when using multiple adapters. I forked your test repo, added @date-io/dayjs, and import it then the error was reproduced.

Using multiple adapters is not practical in applications like this though, the critical case is writing libraries that import and reexport the adapters as written in the README. I actually encountered this problem when writing a library.


Here is the diff representing what I added: https://github.com/dmtrKovalenko/test-date-io-type/compare/master...whitphx:typing-bug

The error log is below.

$ yarn tsc
yarn run v1.22.17
$ /workspace/test-date-io-type/node_modules/.bin/tsc
node_modules/@date-io/date-fns/type/index.d.ts:2:15 - error TS2300: Duplicate identifier 'DateType'.

2   export type DateType = Date;
                ~~~~~~~~

  node_modules/@date-io/dayjs/type/index.d.ts:4:15
    4   export type DateType = Dayjs;
                    ~~~~~~~~
    'DateType' was also declared here.

node_modules/@date-io/dayjs/build/dayjs-utils.d.ts:1:26 - error TS2307: Cannot find module 'dayjs'.

1 import defaultDayjs from "dayjs";
                           ~~~~~~~

node_modules/@date-io/dayjs/type/index.d.ts:2:25 - error TS2307: Cannot find module 'dayjs'.

2   import { Dayjs } from "dayjs";
                          ~~~~~~~

node_modules/@date-io/dayjs/type/index.d.ts:4:15 - error TS2300: Duplicate identifier 'DateType'.

4   export type DateType = Dayjs;
                ~~~~~~~~

  node_modules/@date-io/date-fns/type/index.d.ts:2:15
    2   export type DateType = Date;
                    ~~~~~~~~
    'DateType' was also declared here.

Found 4 errors.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
LukasTy commented 1 year ago

@dmtrKovalenko Is there a reason for having the declared modules with the same name? Having each module with a unique name should avoid the error and from the looks of the source code—shouldn't impact anything. 🤔 Here is a diff of the change I'm suggesting:

diff --git a/packages/date-fns-jalali/type/index.d.ts b/packages/date-fns-jalali/type/index.d.ts
index 983d68a..8298607 100644
--- a/packages/date-fns-jalali/type/index.d.ts
+++ b/packages/date-fns-jalali/type/index.d.ts
@@ -1,3 +1,3 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/date-fns-jalali" {
   export type DateType = Date;
 }
diff --git a/packages/date-fns/type/index.d.ts b/packages/date-fns/type/index.d.ts
index 983d68a..9f6860b 100644
--- a/packages/date-fns/type/index.d.ts
+++ b/packages/date-fns/type/index.d.ts
@@ -1,3 +1,3 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/date-fns" {
   export type DateType = Date;
 }
diff --git a/packages/dayjs/type/index.d.ts b/packages/dayjs/type/index.d.ts
index 006c7b5..734f41e 100644
--- a/packages/dayjs/type/index.d.ts
+++ b/packages/dayjs/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/dayjs" {
   import { Dayjs } from "dayjs";

   export type DateType = Dayjs;
diff --git a/packages/hijri/type/index.d.ts b/packages/hijri/type/index.d.ts
index 49d51ac..502b4d7 100644
--- a/packages/hijri/type/index.d.ts
+++ b/packages/hijri/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/moment-hijri" {
   import { Moment } from "moment-hijri";

   export type DateType = Moment;
diff --git a/packages/jalaali/type/index.d.ts b/packages/jalaali/type/index.d.ts
index 8641b03..1355159 100644
--- a/packages/jalaali/type/index.d.ts
+++ b/packages/jalaali/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/moment-jalaali" {
   import { Moment } from "moment-jalaali";

   export type DateType = Moment;
diff --git a/packages/js-joda/type/index.d.ts b/packages/js-joda/type/index.d.ts
index 8276595..f6500f8 100644
--- a/packages/js-joda/type/index.d.ts
+++ b/packages/js-joda/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/@js-joda" {
   import { Temporal } from "@js-joda/core";

   export type DateType = Temporal;
diff --git a/packages/luxon/type/index.d.ts b/packages/luxon/type/index.d.ts
index 6288b4e..ba22831 100644
--- a/packages/luxon/type/index.d.ts
+++ b/packages/luxon/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/luxon" {
   import { DateTime } from "luxon";

   export type DateType = DateTime;
diff --git a/packages/moment/type/index.d.ts b/packages/moment/type/index.d.ts
index 93e75db..ed41a0a 100644
--- a/packages/moment/type/index.d.ts
+++ b/packages/moment/type/index.d.ts
@@ -1,4 +1,4 @@
-declare module "@date-io/type" {
+declare module "@date-io/type/moment" {
   import { Moment } from "moment";

   export type DateType = Moment;
dmtrKovalenko commented 1 year ago

There was some bug in early typescript that I tried to leverage to get automatic inferring for end user but it was resolved. So now we probably should stick with the unique names if that's not a breaking change

LukasTy commented 1 year ago

There was some bug in early typescript that I tried to leverage to get automatic inferring for end user but it was resolved.

I was just looking into possible solution of how can we stop using skipLibCheck: "true" on @mui/x-date-pickers package, because it causes some issues for us authoring our own .d.ts files and one of the issue is that the DateType type is duplicated.

So now we probably should stick with the unique names if that's not a breaking change

It would seem that it would be a breaking change, because the current approach allows users to do the following regardless of the used adapter.

import type { DateType } from '@date-io/type';

I'd argue that this is somewhat misleading and ambiguous, because the actual resulting type of DateType would be different depending on the used adapter and not the import used. 🙈

But after such change these imports would become unique, i.e. for dayjs adapter:

import type { DateType } from '@date-io/type/dayjs';

It could also be smoother if we'd strip the /type part declared module. This would result in import's of the DateType from the same path as the adapters themselves.

import type { DateType } from '@date-io/dayjs';
LukasTy commented 1 year ago

It would not solve our issue completely, because we'd still have to think of a way to deal with similar issue produced by the date-fns/typing.d.ts:

Screenshot 2023-02-02 at 16 55 03

But I do feel that it would still be a step in the right direction for the library. 🤔