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

Date.getMonth could return a union of all possible values #36401

Closed
5 tasks done
nikeee opened this issue Jan 24, 2020 · 8 comments
Closed
5 tasks done

Date.getMonth could return a union of all possible values #36401

nikeee opened this issue Jan 24, 2020 · 8 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@nikeee
Copy link
Contributor

nikeee commented Jan 24, 2020

Search Terms

Suggestion

According to the MDN docs, Date.getMonth() always returns a number between 0 and 11 (inclusive):

An integer number, between 0 and 11, representing the month in the given date according to local time. 0 corresponds to January, 1 to February, and so on.

Currently, the lib.d.ts definition states that this function returns number.

So I suggest du change the definition of getMonth() from:

TypeScript/lib/lib.es5.d.ts

Lines 750 to 751 in 91ffa1c

/** Gets the month, using local time. */
getMonth(): number;

...to something like:

interface Date {
    // ...
    /** Gets the month, using local time. */
    getMonth(): 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11; // (or using a type definition for this union)
    // ...
}

Use Cases

This would be beneficial if user code contains something like if (date.getMonth() == 12). The compiler can then infer that the entire if branch is dead code.
Also, it serves as a quick documentation to let the user know that the return value is in fact a 0-based (not 1-based) month. The number of months in a year is also unlikely to change as well as the base (0/1) of the month index that this function returns.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code <-- I'm not sure about that. It should not be breaking, since 0 | ... | 11 is a subset of number.
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

Remarks

This could likewise be done for getUTCMonth().

There are other methods like getMinutes() and getSeconds(). As they return something in the range of 0 to 60, could also be modeled using a union. However, I think that a large union like this may be considered differently.

Also, there are setters for these values (like setMonth()). Changing the parameter of this function to take a 0 | ... | 11 would actually be a breaking change. It would not reflect the runtime behaviour. setMonth(15) would be a valid statement that will increment the year by 1 and the months by 3. So the setters should be kept as using number.

If this is up for grabs, I'd take this.

@jcalz
Copy link
Contributor

jcalz commented Jan 24, 2020

I can imagine this being a breaking change for code that does anything like this:

let month = new Date().getMonth();
// next month
month = (month + 1) % 12; // error!
//~~~ <-- number not assignable to 0 | ... | 11

but I don't know how prevalent that would be in practice. Could be related to #15645 and/or #26382.

@AnyhowStep
Copy link
Contributor

We don't have NaN type literals yet.
new Date(NaN).getMonth() is NaN.

@jcalz
Copy link
Contributor

jcalz commented Jan 24, 2020

NaN type literals

I guess #28682 is currently the canonical issue for that.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Jan 24, 2020
@nikeee
Copy link
Contributor Author

nikeee commented Jan 24, 2020

We don't have NaN type literals yet.

Well, then we are not able to map the types properly to the runtime behavior.

Even if we had the NaN type, returning NaN | 1 | ... | 11 would even be misleading in the most places. Most NaN checks would likely be unnecessary.

So at the moment, this may not be possible without sacrificing correctness or (if NaN would be available) usability.

Related issue is numeric ranges: #29119

@RyanCavanaugh
Copy link
Member

I don't think we'd take this without further feedback since it's possibly a breaking change and might interact badly with conditional types. If numeric range types ever happen, that'd be a much better fit.

@nikeee nikeee closed this as completed Jan 24, 2020
@AnyhowStep
Copy link
Contributor

Well, we have ReadonlyArray<> and Array<>.

I don't see why we can't have "regular" Date (where Date.getTime() is possibly NaN) and something like DefinedDate (where DefinedDate.getTime() is not NaN).

DefinedDate.getMonth() could be 0|1|...|10|11.
While Date.getMonth() could be NaN|0|1|...|10|11.

That would require a type guard/assertion to go from Date to DefinedDate, though.
!isNaN(myDate.getTime())

@RyanCavanaugh
Copy link
Member

If we had literally everything we could have, the language would be impossibly complex. So we need to have the things we need, not just the things that are possible.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jan 24, 2020

Well, maybe not part of TS officially.

But someone could just drop the following into their project,

interface DefinedDate extends Date {
  getMonth () : 0|1|/*snip*/|10|11;
}
function assertDefinedDate (d : Date) : asserts d is DefinedDate {
  if (isNaN(d.getTime())) {
    throw new TypeError(`getTime() is NaN`);
  }
}

declare const d : Date;
assertDefinedDate(d);
//0|1|...|10|11
const month : d.getMonth();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants