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

Adds a TypeScript overview page #6120

Merged
merged 24 commits into from Aug 9, 2023
Merged

Conversation

orta
Copy link
Contributor

@orta orta commented Jun 28, 2023

Re: #5960

Adds a 1st draft of a React + TypeScript page. Covers:

  • Describing React Components
  • Hooks, with a special focus on first showing how the inference works and then how to override it
  • Using common React types for Event handling, describing children and typing style
  • Jump off points for deeper understanding

I based this from @markerikson's list in #5960 but also went through a few of my react codebases and looked at what React. types had been used which I think gives a pretty pragmatic set to work from. Open to opinions though!

I punted on any sort of "setting up" style documentation, but opened with a "all the the things we currently recommend have TypeScript documentation" style intro to effectively say "go read the TS docs for your runtime" to get set up. Seemed like a reasonable trade-off as keeping something like that up-to-date is always going to be a thing.

As there is no type-checking with Sandpack, I added a feature to the Sandpack component to open any .tsx files in the TypeScript playground. It only shows on *.tsx files, of which there are no others. You'll also see a little hack in each sandpack example to make it render the tsx file instead of app.js.

It is worth noting that the Playground is going to be getting a reduced feature set over time ( microsoft/TypeScript-Website#2804 ) but that it's very unlikely to be removing the set of features used in this document (JSX support, type-acquisition, URL hash links.) As a precaution, I have been working with the owner of a popular typescript link shortener to host a fully-featured copy of the current playground outside of the TypeScript website gillchristian/tsplay.dev#115 as a backup which URLs could be switched to in the future.

And thanks to @CalleEklund for taking a stab at this earlier too!

Preview

@github-actions
Copy link

github-actions bot commented Jun 28, 2023

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

🎉 Global Bundle Size Decreased

Page Size (compressed)
global 103.14 KB (-4 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Three Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/404 75.36 KB (🟡 +17 B) 178.51 KB
/500 75.36 KB (🟡 +17 B) 178.5 KB
/[[...markdownPath]] 76.82 KB (🟡 +17 B) 179.96 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@harish-sethuraman harish-sethuraman requested review from gaearon and rickhanlonii and removed request for gaearon June 28, 2023 09:18

### `useRef` {/*typing-useref*/}

The [`useRef` hook](/reference/react/useRef) returns a mutable ref object whose `.current` property is initialized to the passed argument. The returned object will persist for the full lifetime of the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably important to explain that useRef<Value> will give you an immutable ref, and useRef<Value | null> (or an initial value) will give you a mutable one.

Reference: https://github.com/typescript-cheatsheets/react#option-2-mutable-value-ref

Copy link
Contributor

Choose a reason for hiding this comment

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

This is 100% the weirdest part about the React types and definitely deserves proper attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll internalize and add

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 think I've covered this distinction (and what its effects are) in a reasonable way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me :)

@phryneas
Copy link
Contributor

I love this! I left some nitpicks, but generally: 💯!


</Sandpack>

This technique works when you have an obvious default value - but often with context you do not, and [in those cases `null` is an appropriate default value](/reference/react/useContext#specifying-a-fallback-default-value). However, you likely would not want `null` to be in the type when consuming the type, our recommendation is to have the hook do a runtime check for it's existence and throw an error when not present:
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is IMO an anti-pattern - if it's included it should at least be marked as such. That's why the doc that's linked is specifically talking about specifying a default value other than null.

I think this can be done pretty simply by re-iterating what the linked doc says eg.

Often, instead of null, there is some more meaningful value you can use as a default. For the occasions where there is not...

Copy link
Contributor Author

@orta orta Jun 28, 2023

Choose a reason for hiding this comment

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

There's a very real chance this is just me - but in the current codebase I work in there are zero cases where context has a default value (there's also a lot of {} as any or null as any) - in some of the Artsy codebases I looked at it was about half of the createContext had a value like those too. The only createContext in this codebase also does it.

I'm sure I can go back and maybe add a fake object which conforms to the same shape with NOOP funcs etc, but doing all that dancing for no useful value seems like busywork vs this technique which raises in the right location when there's an issue and acts like it is supposed to act inside components

But I agree, there's a contradiction in what this side is saying vs what the docs maybe there's a way to re-frame this paragraph 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience it's a very prevalent anti pattern 😅 if it counts for anything pretty sure I've seen React core team members say similar things about context before, but I don't want to derail this discussion too much about context idioms 😆

It's a pattern common enough to be documented here, just needs to match the tone of "this is probably not a good thing to be doing" IMO.

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 think I've toned it down a bit, might be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!


### DOM Events {/*typing-dom-events*/}

When working with DOM events in React, the type of the event is inferred from the event handler. However, when you want to extract a function to be passed to an event handler, you will need to explicitly set the type of the event.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to explain how you choose the event type ie. Why ChangeEvent? And why HTMLInputElement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think this makes sense 👍🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra attention to target vs currentTarget would be nice since this is a recurring issue where people think target should be T instead of just EventTarget for every event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eps1lon can you please re-explain this point? I'm not quite sure I get the distinction on target vs currentTarget enough to get the value

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should do this in a follow-up since it depends on the event. It doesn't block this from being merged.

Choose a reason for hiding this comment

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

Can we juste use the type const handleChange: ComponentProps<'input'>['onChange'] = (e) => {} instead manualy typing all args ?

Co-authored-by: Tom Sherman <the.tomsherman@gmail.com>
Co-authored-by: Lenz Weber-Tronic <mail@lenzw.de>
@markerikson
Copy link
Contributor

At first glance this looks fantastic!

Will come back and give a proper review later

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Really nice. Has the right balance between too much and too little types.

src/content/learn/typescript.md Outdated Show resolved Hide resolved
src/content/learn/typescript.md Outdated Show resolved Hide resolved

`useRef` is often used to access the underlying DOM element of a component, but it can also be used to store any mutable value.

When interacting with the DOM, the type of the ref should be set to the type of the underlying DOM element. The naming rule is `HTML` + the name of the element + `Element`, for example `HTMLDivElement` or `HTMLButtonElement`. You can see the full list from TypeScript 5.1 [here](https://github.com/microsoft/TypeScript/blob/a3773ec590c4f0308d546f0e65818cd0d12402f3/src/lib/dom.generated.d.ts#L26899-L27012). These are provided globally by the "DOM" lib, which is included by default in TypeScript projects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should rather link to MDN or some other DOM documentation. TS provides the declarations. But what interfaces there are is not a TS thing but a HTML thing.

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 agree with this, I did look and https://developer.mozilla.org/en-US/docs/Web/HTML/Element was the best list of the element types but didn't contain the actual names. This glossary is ok https://developer.mozilla.org/en-US/docs/Web/API#h_2 but not a great thing to link to as it's got a bunch of extras in it

The TypeScript link provided is really good because it provides a list of "a": HTMLAnchorElement which takes the thing you know and shows you the thing you need to figure out.

I think these some word-smithing which could highlight how these are named based on the Web APIs though

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've re-framed this section a bit (more "this is the DOM naming system", not "it kinda works like this", and also linked to the MDN source of them

src/content/learn/typescript.md Outdated Show resolved Hide resolved
@orta
Copy link
Contributor Author

orta commented Jul 3, 2023

OK, this is good for a 2nd read

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I think this is a really great start. Thank you!

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Great start! I have a few comments!


<Note>

These sandboxes can handle TypeScript code, but they do not run the type-checker. This means you can amend the TypeScript sandboxes to learn, but you won't get any type errors or warnings. To get type-checking, you can use the [TypeScript Playground](https://www.typescriptlang.org/play) or use a more fully-featured online sandbox.
Copy link
Member

Choose a reason for hiding this comment

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

What do we need in order to support this? It would be nice to have an example here where the type is wrong to see the benefit.

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 did look into it, and it's possible, codesandbox/sandpack#237 (reply in thread) preview: here https://codesandbox.io/s/github/danilowoz/sandpack-tsserver

It is a bit of a trade-off though, TypeScript is a ~6mb download for the client (.d.ts files and the compiler) and it's something that will occasionally want version bumping to keep up-to-date with the typescript runtime (though it could try re-use the version used by this repo) - all that for a few code samples is generally not worth it IMO.

For the TS website, I hardcoded all this information at build time by making https://shikijs.github.io/twoslash/ - but those code samples are static which I think is a bit restrictive WRT how the rest of the site works?

src/content/learn/typescript.md Outdated Show resolved Hide resolved
src/content/learn/typescript.md Outdated Show resolved Hide resolved
src/content/learn/typescript.md Show resolved Hide resolved

## Hooks {/*typing-hooks*/}

The type definitions from `@types/react` include types for the built-in hooks, so you can use them in your components without any additional setup. They are built to take into account the code you write in your component, so you will get inferred types a lot of the time and ideally do not need to handle the minutiae of providing the types. However, we can look at a few examples of how to provide types for hooks.
Copy link
Member

Choose a reason for hiding this comment

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

If you're new to typescript, it's probably not clear what "inferred" means. What about a Deep Dive section here describing this?

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 like this idea!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also added a link to https://www.typescriptlang.org/docs/handbook/type-inference.html here. We call out how inferred types behave on the specific hook sections. Should we explain inference more here?

Copy link
Member

Choose a reason for hiding this comment

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

We can follow up with the DeepDive section.

src/content/learn/typescript.md Show resolved Hide resolved
src/content/learn/typescript.md Show resolved Hide resolved
src/content/learn/typescript.md Show resolved Hide resolved
src/content/learn/typescript.md Outdated Show resolved Hide resolved
}
```

### `useRef` {/*typing-useref*/}
Copy link
Member

Choose a reason for hiding this comment

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

Refs and effects are "escape hatches" in the learn docs so let's skip them?

Copy link
Contributor Author

@orta orta Jul 24, 2023

Choose a reason for hiding this comment

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

I agree, and I think this can be a React team decision which I'll be happy with. I have no pushback on removing useEffect as it's really only there because it would be the only one not there from the hooks docs.

useRef though has this tricky "either it's mutable, or it's not" thing that you kinda have to learn or it doesn't really fit in your head when you first try it - because for using it inline in JSX you need to really have that |null . I think beginners get stuck on this bit, and "react typescript useRef" finding this link would describe everything they need to know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably best add to the useRef docs in a "TS usage" section or something. At least for the APIs we consider escape hatches.

Copy link
Contributor

@phryneas phryneas Jul 26, 2023

Choose a reason for hiding this comment

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

Tbh., it would feel very weird if this was the one hook where the TS instructions would be buried somewhere else entirely. In that case, TS usage instrucations would need to be added to pretty much every single API page to keep things kinda consistent.

To keep the scope of this manageable - maybe it could stay here at the bottom of the page under "other hooks"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this page will be digestable if we add TS usage instructions for every API. Mid-term, React with TypeScript will fit naturally into docs and won't be an after-thought that a single page would imply. This page is really just to get you started.

I'm fine if this is just temporary until we figure out TS in demos but if the intention is to keep TS in a single page, I'd like to discuss this a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see TS everywhere in the docs, and then no complaints from me - but so far it seemed like a very hard thing to have any form of TS in the docs, so I didn't expect there to be a high chance for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moved useRef to its reference page while mentioning at the bottom that individual pages may contain TS usage if necessary. I removed the useEffect section since it didn't seem like it contained relevant information.
c0e6963 (#6120)

Copy link
Member

Choose a reason for hiding this comment

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

To unblock, I removed the useRef docs. We can follow up to add it back, but it's weird to merge this with either useRef listed here without all the other more common hooks, or for it to be the only reference guide with TS examples. Let's discuss how best to handle separately.

Co-authored-by: Ricky <rickhanlonii@gmail.com>
Sebastian Silbermann added 4 commits August 7, 2023 18:08
We already say "cause" which makes "knock-on" a bit redundant
dropped useEffect since there's nothing specific about this hook.
@orta
Copy link
Contributor Author

orta commented Aug 8, 2023

Thanks @eps1lon! ( Sorry been really heads down the last few weekends in a row )

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

This is a great start, let's ship this initial version and iterate as needed.

@rickhanlonii rickhanlonii merged commit f82f392 into reactjs:main Aug 9, 2023
4 checks passed
maxjacobson added a commit to maxjacobson/react.dev that referenced this pull request Aug 16, 2023
While reading the new typescript docs introduced in reactjs#6120, I noticed this link points to a fork. This commit changes it to a relative link so it should work everywhere.
maxjacobson added a commit to maxjacobson/react.dev that referenced this pull request Aug 21, 2023
While reading the new typescript docs introduced in reactjs#6120, I noticed this link points to a fork. This commit changes it to a relative link so it should work everywhere.
mattcarrollcode pushed a commit that referenced this pull request Sep 5, 2023
While reading the new typescript docs introduced in #6120, I noticed this link points to a fork. This commit changes it to a relative link so it should work everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants