Initial work to make plugins type-safe (typescript)

[bengry]

All changes are in types only, and and thus not breaking changes as far as runtime is concerned.

The changes are essentially an implementation of option 3 from my comment in #364.

Do note that it will break plugins that reference PluginFunc, since it now requires a generic (this can be avoided with default generics, but there's a good point not to give defaults in this case. I can expand on this if needed).

I also made a small change to the type of UnitType to allow more units to be passed in, seeing that the source allows it. I can pull that into a separate PR if needed.

Lastly, I moved the index.d.ts file into the /src folder, for easier automatic detection by tsc inside the repo. This is unsolved (together with the definition file for the relativeTime plugin I made as an example. Both of these files should be copied over and included in the final bundle.

Update: adding types for so-called "static plugins" is now possible (like the customParseFormat plugin, which modifies the dayjs namespace, and not the prototype). There is a little overhead for plugin type authors, but it's very minimal. See the type for customParseFormat plugin as an example.

[codecov-io]

Codecov Report

Merging #463 into dev will not change coverage.
The diff coverage is n/a.

@@         Coverage Diff         @@
##            dev   #463   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        58     58           
  Lines       511    511           
  Branches     85     85           
===================================
  Hits        511    511

---

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 d4cd3ad...f79c6a9. Read the comment docs.

[bengry]

There's an issue with the way the type is defined at the moment. I'm going to work on that and reopen it when it works.

[iamkun]

Thanks, I'll need some time learning some TS declaration file basic concept with the help of your PR, and reviewers welcome.

[dockleryxk]

Not sure if this helps, but according to this doc something like export as namespace dayjs; could work.

[bengry]

Not sure if this helps, but according to this doc something like export as namespace dayjs; could work.

To do what? Seems like it's for global variables, which dayjs is not (at least under usual circumstances).

[dockleryxk]

Seems like it's for global variables, which dayjs is not (at least under usual circumstances).

That is a misunderstanding that I had then. Throw my comment in the "unhelpful" bucket in that case.

[iamkun]

I have an interesting finding.

While using dayjs v1.8.5 in a typescript, and we want to use pluginrelativeTime, just create a file /node_modules/dayjs/plugin/relativeTime.d.ts with the following code:

import { PluginFunc } from 'dayjs'

declare const plugin: PluginFunc
export = plugin

declare module 'dayjs' {
  interface Dayjs {
    fromNow(withoutSuffix?: boolean): string
    from(compared: any, withoutSuffix?: boolean): string
    toNow(withoutSuffix?: boolean): string
    to(compared: any, withoutSuffix?: boolean): string
  }
}

and in the project app.ts, every thing looks fine:

import * as dayjs from 'dayjs';
import * as relativeTime from 'dayjs/plugin/relativeTime';  // Won't throw an error with no declaration

dayjs.extend(relativeTime);  // 😭However, no matter this line is called or not, everything will be fine  

console.log('dayjs: ', dayjs().fromNow()); // type assertion works as expected

[bengry]

@iamkun This was one of my issues with module augmentation, as stated here (option 5).
It works, but the issue is that it works all the time, giving the impression that some methods are available, when in fact they are not. That's especially problematic when starting out with a library, before you know the full API and how it works.
That's why I prefer the solution suggested and implemented in this PR - it works well with types, while requiring minimal additions from the user.
In addition, one this not demonstrated in the PR tests is that once you do call .extend(plugin), since no JS changed - the prototype does still change, you can just cast the regular dayjs object you get from import dayjs from 'dayjs' to a Dayjs<PluginA & PluginB...> to get better type annotations in-place.

[iamkun]

Oh I see. I didn't know this is called module augmentation.
In my test, only we added this line could make the plugin types work.

import * as relativeTime from 'dayjs/plugin/relativeTime';

[bengry]

@iamkun Yea, this does work, but doesn't offer any type-safety as far as the actual extend call goes. With this PR you could either re-export a dayjs instance after calling extend, and be sure that you got the correct plugins configured (using type inference), or cast it yourself to the appropriate plugin, which means you're already telling the compiler "hey, I know what I'm doing - trust me on this type".

/* Option #1 - re-export and get type safety through type inference */
// config/dayjs.ts
import dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';

export default dayjs.extend(relativeTime);

// some-other-file.ts
import dayjs from '../config/dayjs';

dayjs().toNow(); // no error since dayjs is inferred to be of type Dayjs<RelativeTimePlugin>


/* Option #2 - cast it yourself */
// config/dayjs.ts
import dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';

// just making the call here to mutate the prototype
dayjs.extend(relativeTime);

// some-other-file.ts
import dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';

dayjs().toNow(); // type error
(dayjs() as Dayjs<RelativeTimePlugin>).toNow() // no type error - though you are telling the compiler to trust you on the type here.

Is there anything blocking this PR or that requires further discussion?

[iamkun]

I can't make this pr work on my side.

I've copied the code to index.d.ts and /plugin/repativeTime.d.ts.

And in my ts project

import * as dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';

dayjs.extend(relativeTime);

console.log(dayjs().fromNow()); // error here, Property 'fromNow' does not exist on type '{}'. 

[bengry]

@iamkun what do you mean by your side? I'd appreciate it if you could send a link to a repo with the current state you're in. I'll try to see if there's anything missing, on either end.

[iamkun]

@bengry check the invitation please

[bengry]

@iamkun I looked at the code, and pushed to a new branch there (fixed) with the way it should be used.

[iamkun]

@bengry Oh, thanks.
Finally got what you mean of re-export a dayjs instance after calling extend. And thanks for your PR.

//  /utils/dayjs.js
import * as dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';
export const customDayjs = dayjs.extend(relativeTime);
export default customDayjs;
// and import this ./utils/dayjs else where in your project

However, it looks a little bit wired to me and a bit complicated. I think module augmentation might more easy to understand.

We might not need that strict type checking, right?

[bengry]

I agree that this is more convoluted then module augmentation, but it's the best thing I could think of but being type-safe.
Because you still need to call extend to make the actual changes (and not something like how d3/pollyfills work - where you just do import 'my-package' somewhere in your code, I think this is better. The downside is pretty small IMO - you just need to do a find & replace operation in your code base for import dayjs from 'dayjs' to import dayjs from 'some/custom/location'.

I think we may be able to get this working alongside module augmentation (so you can choose how to go about doing this), but this requires further exploration, and may require separating the dayjs plugins into separate packages (one per plugin), to allow it to be really seamless, which would be a breaking change - something I wanted to avoid so far.

What do you think?

[iamkun]

Yes, I know it's better than module augmentation of being type safe. And we really can't check whether extend is called or not with module augmentation.

But, to me, I think Day.js is just a tiny simple date-time utils library, you could see this from its size. And I really don't want our user to create a separate file to make everything work on their project for this small library. That is to say, it shoud be a out of the box solution. That's what a lib should really do.

How do you think?

[bengry]

While I do see this as a library that could be used in large apps, I get what you're saying.
I'll see if I can make this work with module augmentation in addition to the above - so you could choose to use either approach when building an app with day.js + TypeScript. One makes sure you're type-safe, while the other is a bit easier to use, especially on smaller apps.

I'm not sure when exactly I'll have time for this, but I'll try to get to it soon.

Would having both approaches (with appropriate docs in the day.js docs) be satisfactory?

[iamkun]

Will it takes you a lot of time? If so, we could use module augmentation first. And wait for the feedback. And we can also get this updated PR merged.

We could finish the full version in two or more PRs, and no need to do everything here.

[bengry]

I'm not sure, since I'd like to get both working with one codebase.
I agree that if the direction is to have module augmentation as an option - we can include it for now, and keep iterating over in more PRs to get the one with extend working.

You can use the definitions I created for some of the plugins here for the module augmentation.

[iamkun]

Agreed, cool.

So, step one, we could start from module augmentation, and make basic plugin type check work.

And step two, make extend working.

If I understand this correctly, I could start step one this weekend with the help of your code in this PR, this comment #463 (comment), and #418. Or you could create a new PR if you want your code get merged. Both are fine to me.

[bengry]

@iamkun #418 looks good overall (there are some unrelated changes there, and the ConfigType -> DateType rename is pretty weird and unneeded) - so lets merge that first like you suggested to make module augmentation working, then I'll take another look at how this PR (#463) can work together with it, and I'll make the appropriate changes.

[ypresto]

ConfigType change was intended.
dayjs() and other methods receive Date, dayjs object, date string and epoch number. They are actually a "date", not config.

Actual config is appear here:

dayjs/src/index.js

Line 37 in f343206

cfg.date = date

It is object passed to Dayjs constructor, which contains date, format and locale.

Though it is already reverted in below commit 😂
bc4383e

[iamkun]

@ypresto We could do the rename later 😂

[ypresto]

I'm +1 for keeping module augmentation only, because place-by-place installation of plugins for strict type-safety is too much for most of users.

#364 (comment)

[iamkun]

@ypresto I do agree with you at this moment.

But we could still wait for feedback after our first module augmentation version release.

[iamkun]

@bengry Just released v1.8.7 with module augmentation. We could wait for feedback and decide what to do next.

[iamkun]

looks we are fine now and can close this PR. Thank you anyway.