X Tutup
The Wayback Machine - https://web.archive.org/web/20210220032044/https://github.com/preactjs/preact-www/pull/718
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

Fix content/en/guide/v10/typescript.md #718

Closed
wants to merge 17 commits into from

Conversation

@38elements
Copy link
Contributor

@38elements 38elements commented Nov 26, 2020

This pull request is for reviewing #716.
Japanese version is added this change to.
I think #703 includes many repeating, incorrect, unnecessary things.

@ddprrt
Copy link
Contributor

@ddprrt ddprrt commented Nov 26, 2020

I think it's not only a type for children that's added to props but also the property children in general. Worth mentioning?

@38elements
Copy link
Contributor Author

@38elements 38elements commented Nov 26, 2020

I think it's not only a type for children that's added to props but also the property children in general. Worth mentioning?

`FunctionComponent` also adds a type for `children`:

Does the original description worth it?

@@ -7,8 +7,6 @@ description: "Preact has built-in TypeScript support. Learn how to make use of i

Preact ships TypeScript type definitions, which are used by the library itself!

When you use Preact in a TypeScript-aware editor (like VSCode), you can benefit from the added type information while writing regular JavaScript. If you want to add type information to your own applications, you can use [JSDoc annotations](https://fettblog.eu/typescript-jsdoc-superpowers/), or write TypeScript and transpile to regular JavaScript. This section will focus on the latter.

This comment has been minimized.

@38elements

38elements Nov 26, 2020
Author Contributor

This is a description of the VSCode features and methods of adding types.
I think it is unnecessary in Preact document.
Therefore, I removed this line.

This comment has been minimized.

@developit

developit Dec 7, 2020
Member

A thought: may we could have this just be a short "tip" like this:

Screen Shot 2020-12-07 at 10 31 46 AM

the markup I used there was:

> 💁 You can also use Preact's type definitions in plain JavaScript with [JSDoc annotations](https://fettblog.eu/typescript-jsdoc-superpowers/).

This comment has been minimized.

@38elements

38elements Dec 9, 2020
Author Contributor

I changed to your suggestion.

@ddprrt
Copy link
Contributor

@ddprrt ddprrt commented Nov 26, 2020

You're right, this might be misleading in the original version as well. What about:

Preact ships a `FunctionComponent` type to annotate anonymous functions. `FunctionComponent` adds a property `children` to the type for `props`:

This should be correct.

@38elements 38elements marked this pull request as draft Nov 27, 2020
@@ -92,7 +90,7 @@ function Greeting({ name = "User" }: GreetingProps) {
}
```

Preact also ships a `FunctionComponent` type to annotate anonymous functions. `FunctionComponent` also adds a type for `children`:
Preact ships a `FunctionComponent` type to annotate anonymous functions. `FunctionComponent` adds a type for `children` to `props`:

This comment has been minimized.

@38elements 38elements force-pushed the 38elements:patch-2020-11-26 branch from 97096b1 to ee367f8 Dec 3, 2020
@@ -170,7 +170,7 @@ class Expandable extends Component<ExpandableProps, ExpandableState> {

## Typing events

Preact emits regular DOM events. As long as your TypeScript project includes the `dom` library (set it in `tsconfig.json`), you have access to all event types that are available in your current configuration.

This comment has been minimized.

@38elements

38elements Dec 4, 2020
Author Contributor

In Preact core, onChange is the standard [DOM change event]

There is "standard DOM event" in other page.
Therefore, I change to "standard DOM event".
Element emits event.

@@ -170,7 +170,7 @@ class Expandable extends Component<ExpandableProps, ExpandableState> {

## Typing events

Preact handles standard DOM events. As long as your TypeScript project includes the `dom` library (set it in `tsconfig.json`), you have access to all event types that are available in your current configuration.

This comment has been minimized.

@38elements

38elements Dec 5, 2020
Author Contributor

lib includes DOM.
standard DOM events types is more explicit.

@@ -187,7 +187,7 @@ export class Button extends Component {
}
```

You can restrict event handlers by adding a type annotation for `this` to the function signature as the first argument. This argument will be erased after transpilation.

This comment has been minimized.

@38elements

38elements Dec 6, 2020
Author Contributor

This content is an explanation about this parameters.
Therefore, This is not appropriate in Preact document.


```tsx
export class Button extends Component {
handleClick(event: MouseEvent) {
event.preventDefault();
if (event.target instanceof HTMLElement) {
alert(event.target.tagName); // Alerts BUTTON

This comment has been minimized.

@38elements

38elements Dec 6, 2020
Author Contributor

Another one uses console.log().
It is inconsistent.

This comment has been minimized.

@developit

developit Dec 7, 2020
Member

Good call!

render() {
return (
<div class="expandable">
<h2>
// `this.props.title` is string type by ExpandableProps.

This comment has been minimized.

@developit

developit Dec 7, 2020
Member

I need to check if our syntax highlighter can handle line comments in JSX

content/en/guide/v10/typescript.md Outdated Show resolved Hide resolved

```tsx
export class Button extends Component {
handleClick(event: MouseEvent) {
event.preventDefault();
if (event.target instanceof HTMLElement) {
alert(event.target.tagName); // Alerts BUTTON

This comment has been minimized.

@developit

developit Dec 7, 2020
Member

Good call!

content/en/guide/v10/typescript.md Outdated Show resolved Hide resolved
content/en/guide/v10/typescript.md Outdated Show resolved Hide resolved
@@ -7,8 +7,6 @@ description: "Preact has built-in TypeScript support. Learn how to make use of i

Preact ships TypeScript type definitions, which are used by the library itself!

When you use Preact in a TypeScript-aware editor (like VSCode), you can benefit from the added type information while writing regular JavaScript. If you want to add type information to your own applications, you can use [JSDoc annotations](https://fettblog.eu/typescript-jsdoc-superpowers/), or write TypeScript and transpile to regular JavaScript. This section will focus on the latter.

This comment has been minimized.

@developit

developit Dec 7, 2020
Member

A thought: may we could have this just be a short "tip" like this:

Screen Shot 2020-12-07 at 10 31 46 AM

the markup I used there was:

> 💁 You can also use Preact's type definitions in plain JavaScript with [JSDoc annotations](https://fettblog.eu/typescript-jsdoc-superpowers/).
@38elements 38elements force-pushed the 38elements:patch-2020-11-26 branch 2 times, most recently from 1d4f948 to 881fe90 Dec 8, 2020
if (event.target instanceof HTMLElement) {
console.log(event.target.localName); // "button"
}
console.log(this.tagName); // "BUTTON"
}

This comment has been minimized.

@38elements

38elements Dec 8, 2020
Author Contributor

I think this example should use this.
Since this is HTMLButtonElement, instanceof is unnecessary.
Another example uses tagName.

This comment has been minimized.

@ddprrt

ddprrt Dec 8, 2020
Contributor

I agree, this is much more concise and showcases this binding much better 👍

ref = createRef<HTMLAnchorElement>();
componentDidMount() {
// current is of type HTMLAnchorElement

This comment has been minimized.

@38elements

38elements Dec 9, 2020
Author Contributor

this.ref.current is div.
This comment is confusing.

This comment has been minimized.

@ddprrt

ddprrt Dec 9, 2020
Contributor

Good catch!

}
}
```

This helps a lot if you want to make sure that the elements you `ref` to are input elements that can be e.g. focussed.

## Typing context

`createContext` tries to infer as much as possible from the intial values you pass to:

This comment has been minimized.

@38elements

38elements Dec 11, 2020
Author Contributor

TypeScript infers, not createContext.

@@ -92,7 +93,7 @@ function Greeting({ name = "User" }: GreetingProps) {
}
```

Preact also ships a `FunctionComponent` type to annotate anonymous functions. `FunctionComponent` also adds a type for `children`:

This comment has been minimized.

@38elements

38elements Dec 12, 2020
Author Contributor

FunctionComponent type corresponds all functional components, not only an anonymous function.
This sentence is confusing.


There are different ways to type components in Preact. Class components have generic type variables to ensure type safety. TypeScript sees a function as functional component as long as it returns JSX. There are multiple solutions to define props for functional components.

This comment has been minimized.

@38elements

38elements Dec 12, 2020
Author Contributor

TypeScript sees a function as functional component as long as it returns JSX.

Unless a function is matched FunctionComponent or FunctionalComponent, it is not considered a functional components.
This is incorrect.

This comment has been minimized.

@38elements

38elements Dec 12, 2020
Author Contributor

Since there is the same content at below, I think this line is unnecessary.

@38elements 38elements force-pushed the 38elements:patch-2020-11-26 branch from 46f98fb to 33650a8 Dec 12, 2020
You can set default props by setting a default value in the function signature.

```tsx
type GreetingProps = {
name?: string; // name is optional!
}
function Greeting({ name = "User" }: GreetingProps) {
// name is at least "User"
return <div>Hello {name}!</div>
}
```
Comment on lines -82 to -93

This comment has been minimized.

@38elements

38elements Dec 12, 2020
Author Contributor

This is explanation for Default parameters.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters
Therefore, this is not appropriate in TypeScript page of Preact document.

This comment has been minimized.

@marvinhagemeister

marvinhagemeister Dec 26, 2020
Member

I think the section was great and we should keep it. People switching from prop types over to TS will naturally ask these questions as most linters treat prop-types and default props as the same. We should cater to those users too 👍

@38elements 38elements force-pushed the 38elements:patch-2020-11-26 branch from 2adbfbb to 01ea56a Dec 12, 2020
@@ -81,7 +81,7 @@ type Props = {
age: number;
};
function MyComponent({ name, age }: Props) {

This comment has been minimized.

@38elements

38elements Dec 12, 2020
Author Contributor

Most React libraries use React.FC.
Using FunctionComponent method is explicit.
Therefore, this example is unnecessary.

<div hidden={this.state.toggled}>{this.props.children}</div>
</div>
);
}
}
```

Class components include children by default, typed as `ComponentChildren`.

## Typing events

This comment has been minimized.

@38elements

38elements Dec 13, 2020
Author Contributor

I think it is necessary to describe about TargetedEvent, since TargetedEvent does not exist in React.

38elements added 2 commits Dec 14, 2020

The `createRef` function is also generic, and lets you bind references to element types. In this example, we ensure that the reference can only be bound to `HTMLAnchorElement`. Using `ref` with any other element lets TypeScript thrown an error:

This comment has been minimized.

@38elements

38elements Dec 15, 2020
Author Contributor

Using ref with any other element lets TypeScript thrown an error.

This is explanation about TypeScript TypeError.
I think it is unnecessary in Preact document.
Therefore, I removed this sentence.

render() {
return <div ref={this.ref}>Foo</div>;
// ~~~
// 💥 Error! Ref only can be used for HTMLAnchorElement

This comment has been minimized.

@38elements

38elements Dec 15, 2020
Author Contributor

This content is repeating.

38elements added 2 commits Dec 16, 2020
38elements added 2 commits Dec 18, 2020
@@ -173,18 +232,17 @@ class Expandable extends Component<ExpandableProps, ExpandableState> {
}
```

Class components include children by default, typed as `ComponentChildren`.

This comment has been minimized.

@38elements

38elements Dec 18, 2020
Author Contributor

children? is default, not children.

type RenderableProps<P, RefType = any> = P & Readonly<Attributes & { children?: ComponentChildren; ref?: Ref<RefType> }>;
@38elements
Copy link
Contributor Author

@38elements 38elements commented Dec 19, 2020

I think it's not only a type for children that's added to props but also the property children in general. Worth mentioning?

I sent this pull request because I thought it was worth it.
Is a person who wrote this existing description a person who deserves to say "Is this worth it?" to others?

@@ -100,7 +100,7 @@ const MyComponent: FunctionComponent<Props> = function ({ name, age }) {
type RenderableProps<P, RefType = any> = P & Readonly<Attributes & { children?: ComponentChildren; ref?: Ref<RefType> }>;
```

When `children` is required, it need to be specified:
Since `children` in `RenderableProps` type is `children?: ComponentChildren`, `children` need to be specified when `children` is required:

This comment has been minimized.

@38elements

38elements Dec 19, 2020
Author Contributor

This clarifies the reason for specifying children.

It also requires you to pass in all the properties you defined in the initial value:

```tsx
function App() {
// This one errors 💥 as we haven't defined theme
return (
<AppContext.Provider
value={{
// ~~~~~
// 💥 Error: theme not defined
lang: "de",
authenticated: true
}}
>
{}
<ComponentThatUsesAppContext />
</AppContext.Provider>
);
}
```
Comment on lines -261 to -280

This comment has been minimized.

@38elements

38elements Dec 20, 2020
Author Contributor

This is mere explanation about TypeScript TypeError.
I think it is unnecessary in Preact document.
Therefore, I removed this sentence.

If you don't want to specify all properties, you can either merge default values with overrides:

```tsx
const AppContext = createContext(appContextDefault);
function App() {
return (
<AppContext.Provider
value={{
lang: "de",
...appContextDefault
}}
>
<ComponentThatUsesAppContext />
</AppContext.Provider>
);
}
```
Comment on lines -282 to -299

This comment has been minimized.

@38elements

38elements Dec 20, 2020
Author Contributor

This is mere explanation about how to pass values to props using destructuring assignment.
I think it is not suitable in this page.
Therefore, I removed this sentence.

Or you work without default values and use bind the generic type variable to bind context to a certain type:

```tsx
type AppContextValues = {
authenticated: boolean;
lang: string;
theme: string;
}
const AppContext = createContext<Partial<AppContextValues>>({});
function App() {
return (
<AppContext.Provider
value={{
lang: "de"
}}
>
<ComponentThatUsesAppContext />
</AppContext.Provider>
);
```

All values become optional, so you have to do null checks when using them.
Comment on lines 301 to 324

This comment has been minimized.

@38elements

38elements Dec 20, 2020
Author Contributor

This is mere explanation about Partial type.
I think it is unnecessary in Preact document.
Therefore, I removed this sentence.

@38elements 38elements changed the title Improve content/en/guide/v10/typescript.md Fix content/en/guide/v10/typescript.md Dec 21, 2020
@@ -350,7 +358,7 @@ const Counter = ({ initial = 0 }) => {

`useEffect` does extra checks so you only return cleanup functions.

This comment has been minimized.

@38elements

38elements Dec 21, 2020
Author Contributor

It seems this line is mere explanation about useEffect. Since Hooks page is suitable, this line is unnecessary.

// ✅ if you return something from the effect callback
// it HAS to be a function without arguments
Comment on lines -360 to -361

This comment has been minimized.

@38elements

38elements Dec 21, 2020
Author Contributor

It seems this lines is mere explanation about useEffect and TypeError.
No one passes an argument since this function does not accept arguments.
Therefore, this lines is unnecessary.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

This PR has gotten a bit out of hand and is impossible to merge. Most of the changes here are made under the assumption that there should be a strict divide between what should be in the TS docs, on MDN or in ours. The line is much more blurry in reality and I vastly prefer the previous approach of including some tips in our docs, even if they can be found in detail in others scattered over the internet.

I know this is just a draft, but we should strife to make smaller PRs. When they are more about corrections they become easier to review too 👍

@38elements
Copy link
Contributor Author

@38elements 38elements commented Jan 11, 2021

@marvinhagemeister
This pull request's first title is "Improve content/en/guide/v10/typescript.md".
This pull request was only one-line change at first.
(This pull request was squashed.)
#718 (comment)
I added fellowing message at first.

This describes what a type for children is added to.
https://github.com/preactjs/preact/blob/f955cfcceb50d504faa7c05a95065512cf900e57/src/index.d.ts#L72-L90

I think this is a looking down way of saying it:

"I think it's not only a type for children that's added to props but also the property children in general. Worth mentioning?"

I can accept the following comments:

"I think it's not only a type for children that's added to props but also the property children in general. Therefore, I think this change is unnecessary."

I have already explained. And I sent it because I think it is worth it. I do not think it is a necessary question.
I think his comments are strange and inconsistent. Only I am commented like this in Preact community.

I think you have not been never commented like this in Preact community.
Have you been commented like this in here?

// Types for props
// in the case `children` is required.

This comment has been minimized.

@38elements

38elements Jan 13, 2021
Author Contributor

This line is unnecessary. I made mistake.

type ExpandableProps = {
title: string;
children: ComponentChildren;

This comment has been minimized.

@38elements

38elements Jan 13, 2021
Author Contributor

This line is unnecessary. I made mistake.

@preactjs preactjs locked as too heated and limited conversation to collaborators Jan 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
X Tutup