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

Add lacking documentation for Date constructor interface #45519

Open
bruno-brant opened this issue Aug 19, 2021 · 9 comments
Open

Add lacking documentation for Date constructor interface #45519

bruno-brant opened this issue Aug 19, 2021 · 9 comments
Labels
Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript

Comments

@bruno-brant
Copy link

lib Update Request

Configuration Check

My compilation target is ES5 and my lib is the default.

Missing / Incorrect Definition

None - this is just improving documentation (jsdocs).

Sample Code

None - this is just improving documentation (jsdocs).

Documentation Link

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

@MartinJohns
Copy link
Contributor

this is just improving documentation (jsdocs).

You're actually introducing a breaking change in your PR... This is not just improving documentation.

@bruno-brant
Copy link
Author

this is just improving documentation (jsdocs).

You're actually introducing a breaking change in your PR... This is not just improving documentation.

What change? Care to point me to it?

@MartinJohns
Copy link
Contributor

The original version has a constructor overload accepting a number | string type. Your changes remove this overload and replaces it with two distinct overloads. This is a breaking change.

@bruno-brant
Copy link
Author

Heck, you're completely right... I was under the impression that Typescript would consider it to be the same 🤦‍♂️. I'll fix it right now.

@bruno-brant
Copy link
Author

@MartinJohns if can spare the time, could you explain why does this change is breaking? I've seem that the tests are failing, but I can't to come up with a sample where the code would break by splitting those methods.

@MartinJohns
Copy link
Contributor

You can't call the constructor anymore with a variable typed as number | string:

const value = 0 as number | string;
new Date(value)

@RyanCavanaugh RyanCavanaugh added Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript labels Aug 19, 2021
@RyanCavanaugh
Copy link
Member

It'd be nice to iterate on these as individual issues, then get a PR to update the text once we've agreed on the phrasing.

@bruno-brant
Copy link
Author

hey @RyanCavanaugh, I don't think the text is great either - I've copied it from MDN though, which seems to me to be an "official enough" source. They are, at the very least, correct.

What do you think: I believe the docs I proposed are better than nothing, so what do you say we approve it as it is and create an issue for improving it later? This way, we get docs asap, and then can work on it with a bit more time.

@bruno-brant
Copy link
Author

And while we're at it, is there a guide for this kind of documentation? I wouldn't mind making those improvements and if there's some guidance I'd be happy to follow it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants