typings: module augmentation and side effects?

[seansfkelley]

moment-range is rather difficult to type accurately. extendMoment is side-effecting and affects the (presumably global) copy of the library you pass it, but the typings mix module augmentation with more-localized changes:

// Global module augmentation, though these values are not actually present if you haven't
// called `extendMoment`.
declare module 'moment' {
  export interface Moment {
    isRange(range: any): boolean;

    within(range: DateRange): boolean;
  }
}

// You get a "local" modified copy of moment, but you've actually changed the globally
// shared copy.
export function extendMoment(momentClass: Moment | typeof moment): MomentRange & Moment;

I've modified this part of the typings in my project such that it only performs module augmentation:

declare module 'moment' {
  export interface Moment {
    isRange(range: any): boolean;

    within(range: DateRange): boolean;
  }

  function range(start: Date, end: Date): DateRange;
  function range(start: Moment, end: Moment): DateRange;
  function range(range: [Date, Date]): DateRange;
  function range(range: [Moment, Moment]): DateRange;
  function range(range: string): DateRange;

  function rangeFromInterval(interval: unitOfTime.Diff, count?: number, date?: Date | Moment): DateRange;
  function rangeFromISOString(isoTimeInterval: string): DateRange;

  // @deprecated 4.0.0
  function parseZoneRange(isoTimeInterval: string): DateRange;
}

I would like to contribute these upstream, but there's a bit of a lie here in that the typings are stating that every copy of moment available anywhere always has the range functions. In practice, this is true, as there is generally only one copy and you'd figure out pretty fast if you forgot to call extendMoment. However, I understand if such a typing lie might not be desirable upstream.

FWIW, moment-timezone, which similarly modifies and returns the moment library, also has the same lie which I've found works great in practice. There's a slight difference in that moment-timezone fetches its own copy of moment rather than having you extendMoment, but both libraries have the same misleading implication that the original moment library is untouched. As such, the typings have chosen to reflect the runtime reality by using augmentation for everything.

There are also a smattering of other typing issues I identified in the process that I would also contribute:

• range accepts a mix-and-match for the two arguments, but the current typings require that the types of the two match resolved
• range accepts nulls/undefined
• MomentRange is far, far too permissive (try calling nonsensical things like moment(1, moment, new Date(), false, moment(), {}, "string", true) in the tests)
• extendMoment should only take a typeof moment, not a Moment resolved

[seansfkelley]

Ping @gf3 who seems to own the types here.

[beaumontjonathan]

Thanks @seansfkelley for the feedback!

Changing how we extend moment would be a huge breaking change and right now can't find a better way. The solution moment-timezone employs is interesting but a core part of moment-range is that we don't want to tie you to a specific version of moment. As moment-timezone is maintained by moment they are in a position to do that, but this is just something we can't do.

If you can think of another way of extending moment without side affects please let us know! 😀

As for the typing issues you've identified - all except for the permissive moment() call have been fixed in the upcoming 4.0.2 release 🎉 If you find a solution for the permissive moment() we would welcome a PR!

[seansfkelley]

To clarify -- I'm not proposing you change how you actually modify the moment instance at runtime, just how you declare the types. With the current types, you can only type-safely get at the range method if you call extendMoment near where you want to use range:

import * as moment_ from "moment";
import { extendMoment } from "moment-range";

const moment = extendMoment(moment_);

(This code may not compile exactly as-is -- I'm doing this from memory.)

This gets tedious if you want to use range in many places. You can sort-of work around it by putting that snippet into its own file and then importing it everywhere, if you're okay with the occasional import moment from "../../../../moment-range";.

However, this usage isn't 100% correct -- moment-range mutates the given moment, so in reality you can just import * as moment from "moment"; anywhere and be done with it, as long as something calls extendMoment before you try to use range. This is how I have changed my fork of the typings. Specifically, I use import * as moment from "moment-timezone"; and get both the timezone + range typings, which is great.

My proposal/my forked typings/the way moment-timezone does it is also not 100% correct, but for what I assume is the common case is much more ergonomic. Either way is a bit of a lie:

• always using extendMoment is not true, because the input moment is mutated
• interface-merging Moment is not true, because if you haven't called extendMoment the methods aren't there

I suppose you could say that this is ultimately caused by the choice to implement extendMoment in the way it is and that that should be resolved -- if it were exclusively side-effecting (i.e. no return value) or didn't mutate the input moment, there would be only one way to express the typings. Just saw the part about having to use side-effects to extend moment.

I was advocating for switching to the more-ergonomic interface-merging declaration under the assumption that it's better to be slightly wrong in that direction than the current direction.

[seansfkelley]

As for the too-permissive moment() declaration, you might be able to

• use Typescript 3.x's support for function-parameter-tuple typing to pull the parameter list from the core moment typings and generate a function signature with that
• copy-paste the declaration from the core moment typings :/

I can't think of any other way to resolve that issue.

[gf3]

an additional consideration is after using the extendMoment() function, depending on the module system, the moment package may not be globally modified. because of this i think it could be misleading to ship an augmented moment interface

[seansfkelley]

The typings as-is are more conservative at the cost of being less ergonomic, so if that's the tradeoff you want to make, I understand. In that case feel free to close this issue if you don't intend on making these changes; I don't see any other potential methods to achieve these improvements without sacrificing that conservativeness.

[gf3]

@seansfkelley another option is to offer an export of moment pre-extended, which could then map to a merged declaration

[beaumontjonathan]

@seansfkelley thanks for your suggestions around the permissive moment(), but I don't think either will work perfectly :/

• use Typescript 3.x's support for function-parameter-tuple typing to pull the parameter list from the core moment typings and generate a function signature with that

We can use Parameters<typeof moment> to pull out the parameter list, but since moment's declaration is overloaded, only the last declaration in the file is pulled out

• copy-paste the declaration from the core moment typings :/

As we don't want to tie users to a specific version of moment, so I'd rather err on the side of permissive types than restrictive

[seansfkelley]

@seansfkelley another option is to offer an export of moment pre-extended, which could then map to a merged declaration

This could work. Are you imagining something along the lines of

import * as moment from "moment-range/extended"; // or whatever it'll be called

and then in extended.d.ts you'd have all the same typings, but with global module augmentation?

I don't know off the top of my head how that would play with

1. non-moment-range imports in other files
   • do they also get the global augmentation? is that desirable? (it would be for me)
2. moment-timezone
   • would you get both augmentations simultaneously?

But it might be worth pursuing.

As we don't want to tie users to a specific version of moment, so I'd rather err on the side of permissive types than restrictive

This makes sense, though with module augmentation you don't need to redeclare the constructor (as I did in the original post). It's probably alright now though, since it doesn't clobber the constructor unless you ask it to with extendMoment.