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

38elements · 2020-11-26T13:33:13Z

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

ddprrt · 2020-11-26T13:35:47Z

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 · 2020-11-26T14:25:48Z

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?

38elements · 2020-11-26T22:11:14Z

content/en/guide/v10/typescript.md

@@ -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 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.

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

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/).

I changed to your suggestion.

ddprrt · 2020-11-26T22:13:08Z

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 · 2020-11-27T10:02:06Z

content/en/guide/v10/typescript.md

@@ -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 describes what a type for children is added to.
https://github.com/preactjs/preact/blob/f955cfcceb50d504faa7c05a95065512cf900e57/src/index.d.ts#L72-L90

38elements · 2020-12-04T12:41:52Z

content/en/guide/v10/typescript.md

@@ -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.


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.

38elements · 2020-12-05T06:15:34Z

content/en/guide/v10/typescript.md

@@ -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.


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

38elements · 2020-12-06T06:45:51Z

content/en/guide/v10/typescript.md

@@ -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 content is an explanation about this parameters.
Therefore, This is not appropriate in Preact document.

38elements · 2020-12-06T06:48:40Z

content/en/guide/v10/typescript.md


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


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

developit · 2020-12-07T15:18:26Z

content/en/guide/v10/typescript.md

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


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

content/en/guide/v10/typescript.md

developit · 2020-12-07T15:22:39Z

content/en/guide/v10/typescript.md


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


content/en/guide/v10/typescript.md

developit · 2020-12-07T15:33:00Z

content/en/guide/v10/typescript.md

@@ -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.


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

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 · 2020-12-08T13:13:05Z

content/en/guide/v10/typescript.md

-    if (event.target instanceof HTMLElement) {
-      console.log(event.target.localName); // "button"
-    }
+    console.log(this.tagName); // "BUTTON"
  }


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

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

38elements · 2020-12-09T14:10:25Z

content/en/guide/v10/typescript.md

  ref = createRef<HTMLAnchorElement>();

-  componentDidMount() {
-    // current is of type HTMLAnchorElement


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

Good catch!

38elements · 2020-12-11T14:12:39Z

content/en/guide/v10/typescript.md

  }
 }
 ```

-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:


TypeScript infers, not createContext.

38elements · 2020-12-11T16:54:20Z

content/en/guide/v10/typescript.md


-### Function components


They are "Functional Components" and "Class Components" in other page.
https://github.com/preactjs/preact-www/blob/922098858eba2769900cc909b18b7b27e5487a49/content/en/guide/v10/components.md#functional-components

38elements · 2020-12-12T02:20:22Z

content/en/guide/v10/typescript.md

@@ -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`:


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

38elements · 2020-12-12T05:36:57Z

content/en/guide/v10/typescript.md


-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.


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.

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

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

38elements · 2020-12-12T09:42:47Z

content/en/guide/v10/typescript.md

-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>
-}
-```


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.

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 · 2020-12-12T12:40:34Z

content/en/guide/v10/typescript.md

@@ -81,7 +81,7 @@ type Props = {
  age: number;
 };

-function MyComponent({ name, age }: Props) {


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

38elements · 2020-12-13T03:51:25Z

content/en/guide/v10/typescript.md

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

-Class components include children by default, typed as `ComponentChildren`.
-
-## Typing events


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

38elements · 2020-12-15T12:43:28Z

content/en/guide/v10/typescript.md


-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:


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.

38elements · 2020-12-15T12:44:42Z

content/en/guide/v10/typescript.md

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


This content is repeating.

38elements · 2020-12-18T15:22:29Z

content/en/guide/v10/typescript.md

@@ -173,18 +232,17 @@ class Expandable extends Component<ExpandableProps, ExpandableState> {
 }
 ```

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


children? is default, not children.

type RenderableProps<P, RefType = any> = P & Readonly<Attributes & { children?: ComponentChildren; ref?: Ref<RefType> }>;

38elements · 2020-12-19T03:10:25Z

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?

38elements · 2020-12-19T12:59:06Z

content/en/guide/v10/typescript.md

@@ -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 clarifies the reason for specifying children.

38elements · 2020-12-20T07:25:01Z

content/en/guide/v10/typescript.md

-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>
-  );
-}
-```


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

38elements · 2020-12-20T07:29:34Z

content/en/guide/v10/typescript.md

-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>
-  );
-}
-```


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.

38elements · 2020-12-20T07:32:19Z

content/en/guide/v10/typescript.md

-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.


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

38elements · 2020-12-21T12:02:57Z

content/en/guide/v10/typescript.md

@@ -350,7 +358,7 @@ const Counter = ({ initial = 0 }) => {

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


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

38elements · 2020-12-21T12:28:08Z

content/en/guide/v10/typescript.md

-  // ✅  if you return something from the effect callback
-  // it HAS to be a function without arguments


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.

marvinhagemeister

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 · 2021-01-11T05:09:04Z

@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?

38elements · 2021-01-13T10:28:35Z

content/en/guide/v10/typescript.md

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


This line is unnecessary. I made mistake.

38elements · 2021-01-13T10:28:45Z

content/en/guide/v10/typescript.md

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


This line is unnecessary. I made mistake.

38elements commented Nov 26, 2020

View reviewed changes

38elements mentioned this pull request Nov 26, 2020

Add content/ja/guide/v10/typescript.md #716

Closed

38elements marked this pull request as draft November 27, 2020 09:57

38elements commented Nov 27, 2020

View reviewed changes

38elements force-pushed the patch-2020-11-26 branch from 97096b1 to ee367f8 Compare December 3, 2020 13:05

38elements commented Dec 4, 2020

View reviewed changes

38elements commented Dec 5, 2020

View reviewed changes

38elements commented Dec 6, 2020

View reviewed changes

developit reviewed Dec 7, 2020

View reviewed changes

38elements force-pushed the patch-2020-11-26 branch 2 times, most recently from 1d4f948 to 881fe90 Compare December 8, 2020 11:56

38elements commented Dec 8, 2020

View reviewed changes

38elements commented Dec 9, 2020

View reviewed changes

38elements commented Dec 11, 2020

View reviewed changes

38elements commented Dec 12, 2020

View reviewed changes

Improve content/en/guide/v10/typescript.md

33650a8

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

38elements force-pushed the patch-2020-11-26 branch from 46f98fb to 33650a8 Compare December 12, 2020 09:29

38elements commented Dec 12, 2020

View reviewed changes

update

01ea56a

38elements force-pushed the patch-2020-11-26 branch from 2adbfbb to 01ea56a Compare December 12, 2020 10:39

update

ac54f2a

38elements commented Dec 12, 2020

View reviewed changes

38elements commented Dec 13, 2020

View reviewed changes

update

781ae68

38elements added 2 commits December 14, 2020 20:32

update

7ddcf5d

update

35dd24a

38elements commented Dec 15, 2020

View reviewed changes

38elements added 2 commits December 16, 2020 20:14

update

47e2cb5

update

099101c

38elements mentioned this pull request Dec 18, 2020

Fix example in content/en/guide/v10/typescript.md #720

Merged

38elements added 2 commits December 19, 2020 00:05

update

181a7df

update

d5edc78

38elements commented Dec 18, 2020

View reviewed changes

update

0a6ee2b

38elements commented Dec 19, 2020

View reviewed changes

update

297fded

38elements commented Dec 20, 2020

View reviewed changes

update

f5e0535

38elements changed the title ~~Improve content/en/guide/v10/typescript.md~~ Fix content/en/guide/v10/typescript.md Dec 21, 2020

38elements commented Dec 21, 2020

View reviewed changes

update

5382742

38elements commented Dec 21, 2020

View reviewed changes

update

b7accfa

marvinhagemeister requested changes Dec 26, 2020

View reviewed changes

marvinhagemeister closed this Dec 26, 2020

38elements mentioned this pull request Dec 28, 2020

Remove .github/CODEOWNERS #727

Merged

38elements commented Jan 13, 2021

View reviewed changes

preactjs locked as too heated and limited conversation to collaborators Jan 13, 2021

		@@ -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.

		@@ -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.

		@@ -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.


		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.


		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:

		@@ -350,7 +358,7 @@ const Counter = ({ initial = 0 }) => {

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

		// ✅ if you return something from the effect callback
		// it HAS to be a function without arguments

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

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

Conversation

38elements commented Nov 26, 2020 • edited Loading

ddprrt commented Nov 26, 2020

38elements commented Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddprrt commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements commented Dec 19, 2020

Choose a reason for hiding this comment

38elements Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

marvinhagemeister left a comment

Choose a reason for hiding this comment

38elements commented Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements commented Nov 26, 2020 •

edited

Loading

38elements commented Nov 26, 2020 •

edited

Loading

38elements Dec 12, 2020 •

edited

Loading

38elements Dec 12, 2020 •

edited

Loading

38elements Dec 20, 2020 •

edited

Loading

38elements Dec 20, 2020 •

edited

Loading

38elements Dec 21, 2020 •

edited

Loading

38elements commented Jan 11, 2021 •

edited

Loading