-
-
Notifications
You must be signed in to change notification settings - Fork 7
Add section for TSX #36
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. It took about an hour for me to go through this PR, because of all the feedback needed on it.
I do appreciate the effort. And I've been through worse early in my time with open source, so I completely understand how it may feel to be suddenly blasted with large amounts of feedback like that.
docs/jsx.md
Outdated
|
||
The simplest way to use JSX is via a [Babel](https://babeljs.io/) plugin. | ||
When using JavaScript, the simplest way to use JSX is via a [Babel](https://babeljs.io/) plugin. When using using [TypeScript](https://www.typescriptlang.org/) follow the [instructions below](#setup-tsx-jsx-in-typescript) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the "when using TypeScript" to the next section.
When using JavaScript, the simplest way to use JSX is via a [Babel](https://babeljs.io/) plugin. When using using [TypeScript](https://www.typescriptlang.org/) follow the [instructions below](#setup-tsx-jsx-in-typescript) | |
When using JavaScript, the simplest way to use JSX is via a [Babel](https://babeljs.io/) plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shortened it. But I do think its wise to link to TSX in this secion. A lot of times the term "JSX" is used for "TSX" interchangably (so is "JavaScript" for "TypeScript") and TypeScript users might stumble over this section and follow the wrong instructions. Tell me if you find that unnecessary and I should fully remove it
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
I incorporated your suggestions. Let me know if you notice other problems / improvements. No worries about the amount of feedback. I do apreciate you being thorough :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the other comments, we avoid unnecessary semicolons in the docs in the same way we do in core. I decided it was better to raise it here than to just spam even more comments (it's already over 20 for this one review).
@@ -58,9 +60,9 @@ m.render(document.body, <MyComponent />) | |||
|
|||
--- | |||
|
|||
### Setup | |||
### Setup JSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TypeScript docs call it JSX, so I'd prefer to stick with that idiom for both.
### Setup JSX | |
### Setup for JavaScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to push this? Also, don't forget to update the TOC.
This setup should be enough to get most JSX functionality working. | ||
|
||
#### Using closure components in TSX | ||
>Because of https://github.com/microsoft/TypeScript/issues/21699, we advise against using [closure components](components.md#closure-component-state) in TypeScript for now. Either use [class components](components.md#class-component-state) without attribute inspection or Hyperscript instead (see the list of alternatives below the code example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to clean this up and elaborate on the recommendation better. Also, we usually keep spaces after Markdown block-level syntax like >
and #
, and this suggestiin factors that in.
If there's anything you feel is unclear or you disagree with, feel free to comment here on it.
>Because of https://github.com/microsoft/TypeScript/issues/21699, we advise against using [closure components](components.md#closure-component-state) in TypeScript for now. Either use [class components](components.md#class-component-state) without attribute inspection or Hyperscript instead (see the list of alternatives below the code example). | |
> Because [TypeScript does not, at the time of writing, allow customizing the type used to check JSX component validity](https://github.com/microsoft/TypeScript/issues/21699), we recommend you avoid using [closure components](components.md#closure-component-state) as JSX component names in TypeScript for now. Either use [class components](components.md#class-component-state) exclusively or use [hyperscript](hyperscript.md) to construct them. | |
> | |
> You'll see what we mean after reading this section. It's not simple to work around TypeScript's missing feature here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with that change but not with your last addition ("You'll see what we mean after reading this section. It's not simple to work around TypeScript's missing feature here").
The actual workaround are only 3 lines of code and its usage is almost identical to normal closure components. I have been working with it for a while now and find it a lot more convenient than using class components.
We might also want to point out that class compoments dont fully work in TSX either (attribute inspection is broken).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with that change but not with your last addition ("You'll see what we mean after reading this section. It's not simple to work around TypeScript's missing feature here").
The actual workaround are only 3 lines of code and its usage is almost identical to normal closure components. I have been working with it for a while now and find it a lot more convenient than using class components.
That's fair. I'd need some more time to think about it. How does ripping out this entire block quote and filing a follow-up issue in this repo sound?
We might also want to point out that class compoments dont fully work in TSX either (attribute inspection is broken).
Good catch. Honestly, I'm convinced the attribute inspection problem may be more of an issue with the current types. Might be better to just file a tracking issue in Mithril core about this. (I've almost moved the types back into Mithril core I don't know how many times by now, because it's just easier to make sure it tracks changes correctly here.)
And if we integrate that here, we could look into what's truly necessary to make Mithril play nice with TypeScript. (I'm leaning towards maybe a @m.component
decorator that just magically fixes up all the types. JS can continue to be free-form, and TS users can benefit from the static annotation.)
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
Co-authored-by: Claudia Meadows <[email protected]>
I completed another round of reworks. I bundled some of the changes into single commits since there would have been a lot of small ones. I also tried to leave comments, that you might have opinions about, unresolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from that discussion, I noticed you missed some stuff from the previous review.
|
||
This setup should be enough to get most JSX functionality working. | ||
|
||
#### Using closure components in TSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In accordance with previous feedback
#### Using closure components in TSX | |
#### Using closure components in TypeScript with JSX |
- [Setup](#setup) | ||
- [Setup JSX](#setup-jsx) | ||
- [Production build](#production-build) | ||
- [Using Babel with Webpack](#using-babel-with-webpack) | ||
- [Setup TSX](#setup-tsx-jsx-in-typescript) | ||
- [Using Closure Components in TSX](#using-closure-components-in-tsx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to update the TOC to reflect the new titles.
This setup should be enough to get most JSX functionality working. | ||
|
||
#### Using closure components in TSX | ||
>Because of https://github.com/microsoft/TypeScript/issues/21699, we advise against using [closure components](components.md#closure-component-state) in TypeScript for now. Either use [class components](components.md#class-component-state) without attribute inspection or Hyperscript instead (see the list of alternatives below the code example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with that change but not with your last addition ("You'll see what we mean after reading this section. It's not simple to work around TypeScript's missing feature here").
The actual workaround are only 3 lines of code and its usage is almost identical to normal closure components. I have been working with it for a while now and find it a lot more convenient than using class components.
That's fair. I'd need some more time to think about it. How does ripping out this entire block quote and filing a follow-up issue in this repo sound?
We might also want to point out that class compoments dont fully work in TSX either (attribute inspection is broken).
Good catch. Honestly, I'm convinced the attribute inspection problem may be more of an issue with the current types. Might be better to just file a tracking issue in Mithril core about this. (I've almost moved the types back into Mithril core I don't know how many times by now, because it's just easier to make sure it tracks changes correctly here.)
And if we integrate that here, we could look into what's truly necessary to make Mithril play nice with TypeScript. (I'm leaning towards maybe a @m.component
decorator that just magically fixes up all the types. JS can continue to be free-form, and TS users can benefit from the static annotation.)
interface Attributes { | ||
greet: string | ||
} | ||
function ChildComponent(vNode: Vnode<Attributes>): m.Component<Attributes> { | ||
return { | ||
view: () => <div>{vNode.attrs.greet}</div> | ||
}; | ||
} | ||
|
||
function ParentComponent() { | ||
return { | ||
view: () => <div> | ||
<ChildComponent greet="Hello World"/> | ||
</div> | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in the previous review.
interface Attributes { | |
greet: string | |
} | |
function ChildComponent(vNode: Vnode<Attributes>): m.Component<Attributes> { | |
return { | |
view: () => <div>{vNode.attrs.greet}</div> | |
}; | |
} | |
function ParentComponent() { | |
return { | |
view: () => <div> | |
<ChildComponent greet="Hello World"/> | |
</div> | |
}; | |
} | |
interface Attributes { | |
greet: string | |
} | |
function ChildComponent(vNode: Vnode<Attributes>): m.Component<Attributes> { | |
return { | |
view: () => <div>{vNode.attrs.greet}</div> | |
} | |
} | |
function ParentComponent() { | |
return { | |
view: () => <div> | |
<ChildComponent greet="Hello World"/> | |
</div> | |
} | |
} |
interface Attributes { | ||
greet: string | ||
} | ||
// We slightly altered the definition of `ChildComponent` by using `TsClosureComponent` | ||
const ChildComponent = TsClosureComponent<Attributes>(vNode => { | ||
return { | ||
view: () => <div>{vNode.attrs.greet}</div> | ||
}; | ||
}) | ||
|
||
function ParentComponent() { | ||
return { | ||
view: () => <div> | ||
<ChildComponent greet="Hello World"/> | ||
</div> | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in the previous review.
interface Attributes { | |
greet: string | |
} | |
// We slightly altered the definition of `ChildComponent` by using `TsClosureComponent` | |
const ChildComponent = TsClosureComponent<Attributes>(vNode => { | |
return { | |
view: () => <div>{vNode.attrs.greet}</div> | |
}; | |
}) | |
function ParentComponent() { | |
return { | |
view: () => <div> | |
<ChildComponent greet="Hello World"/> | |
</div> | |
}; | |
} | |
interface Attributes { | |
greet: string | |
} | |
// We slightly altered the definition of `ChildComponent` by using `TsClosureComponent` | |
const ChildComponent = TsClosureComponent<Attributes>(vNode => { | |
return { | |
view: () => <div>{vNode.attrs.greet}</div> | |
} | |
}) | |
function ParentComponent() { | |
return { | |
view: () => <div> | |
<ChildComponent greet="Hello World"/> | |
</div> | |
} | |
} |
function ChildComponentImpl<T>() { | ||
// ... | ||
} | ||
|
||
const ChildComponent = TsClosureComponent(ChildComponentImpl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in the previous review.
function ChildComponentImpl<T>() { | |
// ... | |
} | |
const ChildComponent = TsClosureComponent(ChildComponentImpl); | |
function ChildComponentImpl<T>() { | |
// ... | |
} | |
const ChildComponent = TsClosureComponent(ChildComponentImpl) |
Edit: missed your comment, sorry. Semicolon's still important, though.
Description
This change adds a new section that describes the setup of TSX (JSX for Typescript) including some additional configurations that should solve all (most?) problems with TSX.
Motivation and Context
There is no official documentation on how to set up JSX in Typescript and other resources often target outdated versions of MithrilJS. There are also a few pitfals that are not trivial to solve.
I wrote up what I have found and implemented for myself. Hopefully this is usefull (and wanted) in the documentation as well :)