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 InstanceType tip in Classes.md #2708

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Conversation

bdombro
Copy link
Contributor

@bdombro bdombro commented Feb 10, 2023

I personally have a hard time finding this type helper (InstanceType) and would love for it to be in the official docs on Classes.

I personally have a hard time finding this type helper (`InstanceType`) and would love for it to be in the official docs on Classes.
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct, see: Playground Link

A variable annotated like const x: Point is already a value of the instance type of Point. InstanceType works on the actual type of the class itself, and that's pretty rare.

I don't think that this part of the page is what should mention InstanceType; we haven't even established what a class can contain or how to use it, let along what an instance is.

@bdombro
Copy link
Contributor Author

bdombro commented May 18, 2023

Oh yeah you caught a typo thanks @jakebailey, should be:

type PointInstance = InstanceType<typeof Point>

Here is how it's useful: Playground LInk

Not sure I understand why not to include it in docs though --is InstanceType an unstable feature? Assuming it is stable, shouldn't we document it somewhere? And would be great to have it on the page with Classes imho

@jakebailey
Copy link
Member

Not sure I understand why not to include it in docs though --is InstanceType an unstable feature? Assuming it is stable, shouldn't we document it somewhere? And would be great to have it on the page with Classes imho

It's certainly stable, yes. It's defined as:

/**
 * Obtain the return type of a constructor function type
 */
type InstanceType<T extends abstract new (...args: any) => any>
	= T extends abstract new (...args: any) => infer R ? R : any;

I'm not against documenting it, but I'm just saying that if we do, it shouldn't come right at the top; we haven't even introduced what classes are and how they work, let alone that they're objects with a new signature. The first (and only) mention of the new signature is down at the bottom.

@bdombro
Copy link
Contributor Author

bdombro commented May 19, 2023

good points. Should we maybe add it at the bottom of Constructor section (link), since InstanceOf returns the type of a constructor function?

@jakebailey
Copy link
Member

I think better would be a section about constructor signatures, placed at least before the mention of new () in the abstract constructor signature section. Whether or not that's a full section, or just a part of the constructor section, I'm not sure. I feel like its own section is preferable.

@bdombro
Copy link
Contributor Author

bdombro commented May 23, 2023

Sure! I updated the PR.

@bdombro
Copy link
Contributor Author

bdombro commented May 23, 2023

@microsoft-github-policy-service agree

@@ -1269,6 +1269,32 @@ const m = new someClass("Hello, world");
// ^?
```

## Class Constructors

Calling a constructor of a class returns an instance, which can be described using the `InstanceType` type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what the right wording is, but I don't think that "calling" is the right one as that can imply Point(1, 2) works (not new Point(1, 2)), even though those are two things. Potentially our docs already describe it as new-ing? Or as just instantiation?

Potentially what this section should be is a description of construct signatures? Or, just a link to https://www.typescriptlang.org/docs/handbook/2/functions.html#construct-signatures

Of course, it's unfortunate that classes and functions are described separately but have this intertwining behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about I change it to "Instantiating a class returns a class instance, ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jakebailey -- following up on this question please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think "Instantiating a class" is the right call probably, there isn't really language like this in the spec nor in MDN with that term, but it's such a globally used term in many languages that I think it fits.

Calling it something like a newable function is a bit more accurate but I think moves too far into 'abstract but correct' terminology

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything I'd go for "instantiating a class produces a class instance", or "The new operator instantiates a class ...", etc.

So like:

Classes are instantiated with new. Given the type of a class itself, the InstanceType utility type will model this operation.

But, I'm a little iffy on this whole PR just from the point of view that it's duplicating content by describing both new (whose docs I linked to), but then also InstanceType, which is described on the utility type page. If anything, I think providing links to each section may be most helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with dropping it if y'all don't want it. Honestly at this point I'm a little exhausted with all the back and forth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that.

If you want something concrete to finish this, then I would probably just use a variation of my text then use []() links to point the word "constructor" to /docs/handbook/2/functions.html#construct-signatures and `InstanceType` to /docs/handbook/utility-types.html#instancetypetype.

You could even just skip the code sample and let those sections speak for themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay! How about now? I accepted your suggestion and tweaked a little to better match those links you shared

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bearing with this.

packages/documentation/copy/en/handbook-v2/Classes.md Outdated Show resolved Hide resolved
@jakebailey jakebailey enabled auto-merge (squash) July 5, 2023 19:34
@jakebailey jakebailey merged commit 2af9258 into microsoft:v2 Jul 5, 2023
5 checks passed
@bdombro bdombro deleted the patch-1 branch July 5, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants