Refactor to use TypeScript `@import` JSDoc tags #2498

remcohaszing · 2024-06-25T13:57:29Z

Initial checklist

I read the support docs
I read the contributing guide
I agree to follow the code of conduct
I searched issues and couldn’t find anything (or linked relevant results below)
If applicable, I’ve added docs and tests

Description of changes

Previously we used @typedef tags to simulate type imports. The problem is that are not really imports. They define and export a new type. We now use @import introduced by TypeScript 5.5 to actually import types.

The pattern using @typedef is still used in index files to export types.

This change also replaces the type casting inside MDX files with definitions of the Props types.

This also replaces usage of the global JSX namespace with proper types. For TypeScript<5.1 the correct return type of React components is ReactElement, for Preact VNode. For TypeScript>=5.1, the correct return type for React components is ReactNode, for Preact ComponentChildren.

ReactElement / VNode and are compatible with TypeScript>=5.1. This is why those are used for public facing APIs, but ReactNode / ComponentChildren is used for internal components.

Previously we used `@typedef` tags to simulate type imports. The problem is that are not really imports. They define and export a new type. We now use `@import` introduced by TypeScript 5.5 to actually import types. The pattern using `@typedef` is still used in index files to export types. This change also replaces the type casting inside MDX files with definitions of the `Props` types. This also replaces usage of the global `JSX` namespace with proper types. For TypeScript<5.1 the correct return type of React components is `ReactElement`, for Preact `VNode`. For TypeScript>=5.1, the correct return type for React components is `ReactNode`, for Preact `ComponentChildren`.

vercel · 2024-06-25T13:57:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 25, 2024 7:52pm

remcohaszing · 2024-06-25T13:59:49Z

docs/_asset/editor.jsx

+ * @import {
+    MdxJsxAttribute,
+    MdxJsxAttributeValueExpression,
+    MdxJsxExpressionAttribute


Using asterisks for multiline imports caused problems, but not in the TypeScript playground. I’m looking into this.

microsoft/TypeScript#59011

I’d work around it by not wrapping 🤷‍♂️

I considered that. It would be ok’ish for this line, but some others would become extremely long. I prefer to keep it like this TBH, keeping the max length at approximately what we use for Prettier.

wooorm · 2024-06-25T14:48:17Z

docs/_asset/editor.jsx

+ * @import {
+    MdxJsxAttribute,
+    MdxJsxAttributeValueExpression,
+    MdxJsxExpressionAttribute


I’d work around it by not wrapping 🤷‍♂️

wooorm · 2024-06-25T14:48:39Z

docs/_asset/editor.jsx

@@ -601,7 +601,7 @@ function ErrorFallback(properties) {
 /**
 * @param {DisplayProperties} properties
 *   Properties.
- * @returns {JSX.Element}
+ * @returns {ReactNode}


better in separate PRs

True. One thing let to another. IMO this is fine to keep as-is now.

wooorm · 2024-06-25T14:50:17Z

docs/blog/index.mdx

+{/**
+  * @typedef Props
+  * @property {Item} navigationTree
+  */}


not 100% on this style? I’d lean to:

{ /** * @import {Item} from '../_component/sort.js' */ /** * @typedef Props * @property {Item} navigationTree */ }

I have given this some thought while working on MDX language server. The indentation of this style feels a bit weird IMO.

{/** * @import {Component} from 'react' * @import {AvatarProps} from './avatar.js' */} {/** * @typedef Comonents * @property {Component<AvatarProps>} avatar */} {/** * @typedef Props * @property {Comonents} components */} {/** * Some description * * @param {unknown} parameter * @returns {undefined} */} export function someFunction(parameter) {} # Hello <Avatar />

But combining all comments, or combining comments arbritratily, with or without indentation feels even more weird to me.

{ /** * @import {Component} from 'react' * @import {AvatarProps} from './avatar.js' */ /** * @typedef Comonents * @property {Component<AvatarProps>} avatar */ /** * @typedef Props * @property {Comonents} components */ } {/** * Some description * * @param {unknown} parameter * @returns {undefined} */} export function someFunction(parameter) {} # Hello <Avatar />

It feels a bit similar to whether or not to group typedefs in a single comment.

So far I feel best about putting each typedef in a separate comment, and each comment in a separate mdxFlowExpression.

Hmm, I feel quite strongly that entering/exiting JS each time is ugly.

And that if you’d put something in there (like a jsx element), that it would be indented 🤔

I’m fine to different typedefs in different comments.

Hmm, I feel quite strongly that entering/exiting JS each time is ugly.

I don’t really mind either style for typedefs. For function docs I would find it weird to combine though, but that’s not applicable in this particular case.

Another thing is that commenting is based on {/* and */} delimiters. Though I don’t think it makes sense to uncomment jsdoc comments.

And that if you’d put something in there (like a jsx element), that it would be indented 🤔

That makes perfect sense. This also reminds me that it’s possible to have multiple comments in one a flow expression, but just one JS expression. JSDoc has meaning, which makes me lean towards preferring one doc per flow expression again. But having the types grouped using one flow expression is also kind of nice.

Anyway, these are all just nitty details. I’ll adjust them to use one MDX flow expression.

For function docs I would find it weird to combine though, but that’s not applicable in this particular case.

Can you clarify what you dislike/like with examples?

Though I don’t think it makes sense to uncomment jsdoc comments.

Agreed. These things are more like type annotations, which currently use special comments, because that’s what’s possible now.

This also reminds me that it’s possible to have multiple comments in one a flow expression, but just one JS expression. JSDoc has meaning, which makes me lean towards preferring one doc per flow expression again. But having the types grouped using one flow expression is also kind of nice.

Right, right, I was thinking about that at first too, but then there’s a difference: these are allowed to be empty! And comments are deemed empty here too. 0 or more empty is still empty. So, zero-or-more empty, then 0-1 expression.

To clarify, some examples of what I think looks good.

Regular comment:

# hi {/* just a smol comment */} hello

More complex expression:

# hi { (function () { return 1 })() } hello

One JSDoc:

{ /** * @import {foo} from 'bar' */ } # hi hello

Multi JSDoc:

{ /** * @import {foo} from 'bar' */ /** * @typedef Options * @property {number} something */ } # hi hello

Multi JSDoc and expression:

# hi { /** * @import {foo} from 'bar' */ /** * @typedef Options * @property {number} something */ (function () { return 1 })() } hello

Also, was just thinking about how do-expressions will be nice!

{ do { if (loggedIn) { <LogoutButton /> } else { <LoginButton /> } } }

Can you clarify what you dislike/like with examples?

If there are multiple exports with JSDoc annotations, It feels weird to me to merge the mdx flow expression the typedefs with that of the JSDoc of the first export.

{ /** * @typedef Props * @property {string} whatever */ /** * @param {string} arg * @returns undefined */ } export function someExport(arg) {} { /** * @param {string} arg * @returns undefined */ } export function anotherExport(arg) {}

Also, was just thinking about how do-expressions will be nice!

Yes, that would be so nice!

ok, I can see this. But that feels like an edge case to me, induced because we allow export statements directly in markdown 🤔

I’m fine with breaking it like:

{ /** * @import {foo} from 'bar' */ /** * @typedef Options * @property {number} something */ } { /** * @param {string} arg * @returns {undefined} */ } export function someExport(arg) {}

wooorm · 2024-06-25T14:53:11Z

packages/mdx/index.js

- * @typedef {import('./lib/evaluate.js').EvaluateOptions} EvaluateOptions
- * @typedef {import('./lib/run.js').RunOptions} RunOptions
+ * @typedef {import('./lib/util/resolve-evaluate-options.js').EvaluateOptions} EvaluateOptions
+ * @typedef {import('./lib/util/resolve-evaluate-options.js').RunOptions} RunOptions
 */


I like an index.d.ts, as it has export type {} from, here it would get rid of the typedef/type alias https://github.com/rehypejs/rehype-document/blob/main/index.d.ts

I’m working on a follow-up PR where I think this is better discussed. I do agree export {} from is an improvement over a typedef.

wooorm · 2024-06-26T10:32:26Z

docs/blog/index.mdx

+   * @typedef Props
+   * @property {Item} navigationTree
+   */
+}


wooorm · 2024-06-26T10:33:05Z

docs/community/index.mdx

+   * @typedef Props
+   * @property {Item} navigationTree
+   */
+}


Like this it’s also more similar to frontmatter 🤔

wooorm · 2024-06-26T10:36:41Z

In this PR, I saw the existing code:

// Register MDX nodes in mdast:
/// <reference types="remark-mdx" />

I think we can now change, and recommend @import {} from 'remark-mdx?

wooorm · 2024-06-26T10:36:45Z

Thanks :)

remcohaszing added 🦋 type/enhancement This is great to have ☂️ area/types This affects typings 👶 semver/patch This is a backwards-compatible fix 🤞 phase/open Post is being triaged manually labels Jun 25, 2024

vercel bot deployed to Preview June 25, 2024 13:57 View deployment

remcohaszing commented Jun 25, 2024

View reviewed changes

wooorm reviewed Jun 25, 2024

View reviewed changes

Group MDX typedefs in single flow expressions

a6b399d

vercel bot deployed to Preview June 25, 2024 19:52 View deployment

wooorm approved these changes Jun 26, 2024

View reviewed changes

wooorm changed the title ~~Use TypeScript @import JSDoc tags~~ Refactor to use TypeScript @import JSDoc tags Jun 26, 2024

wooorm merged commit f12afda into main Jun 26, 2024
10 checks passed

wooorm deleted the type-imports branch June 26, 2024 10:35

wooorm added the 💪 phase/solved Post is done label Jun 26, 2024

This was referenced Nov 11, 2024

[Snyk] Upgrade @mdx-js/loader from 3.0.1 to 3.1.0 Kaylen7/YummyRecipes#5

Open

[Snyk] Upgrade @mdx-js/react from 3.0.1 to 3.1.0 Kaylen7/YummyRecipes#6

Open

qdraw mentioned this pull request Nov 12, 2024

[Snyk] Upgrade @mdx-js/react from 3.0.1 to 3.1.0 qdraw/starsky#1818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use TypeScript `@import` JSDoc tags #2498

Refactor to use TypeScript `@import` JSDoc tags #2498

remcohaszing commented Jun 25, 2024 •

edited

Loading

vercel bot commented Jun 25, 2024 •

edited

Loading

remcohaszing Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 25, 2024

wooorm Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 26, 2024

remcohaszing Jun 27, 2024

wooorm Jun 27, 2024 •

edited

Loading

wooorm Jun 25, 2024

remcohaszing Jun 25, 2024

wooorm Jun 26, 2024

wooorm Jun 26, 2024

wooorm commented Jun 26, 2024

wooorm commented Jun 26, 2024

Refactor to use TypeScript @import JSDoc tags #2498

Refactor to use TypeScript @import JSDoc tags #2498

Conversation

remcohaszing commented Jun 25, 2024 • edited Loading

Initial checklist

Description of changes

vercel bot commented Jun 25, 2024 • 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

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

wooorm Jun 27, 2024 • 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

wooorm commented Jun 26, 2024

wooorm commented Jun 26, 2024

Refactor to use TypeScript `@import` JSDoc tags #2498

Refactor to use TypeScript `@import` JSDoc tags #2498

remcohaszing commented Jun 25, 2024 •

edited

Loading

vercel bot commented Jun 25, 2024 •

edited

Loading

wooorm Jun 27, 2024 •

edited

Loading