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

Docs installation async #136

Closed
wants to merge 23 commits into from
Closed

Docs installation async #136

wants to merge 23 commits into from

Conversation

ChcJohnie
Copy link
Collaborator

Proposal to use RSC async components in MXD components compile list to allow more flexibility inside components for loading files etc.
Tested with DocTabbedCode (wrapper around common TabbedCode which can fetch files from src folder based on path).
In that case mxd markup can be (for List component with 2 files and 1 dependency).
<DocTabbedCode tabs={[ { id: "List.tsx", path: "mailingui/components/list/List.tsx" }, { id: "BulletList.tsx", path: "mailingui/components/list/BulletList.tsx", }, { id: "Paragraph.tsx", path: "mailingui/components/text/Text.tsx", }, ]} />
Off course component could be more or less automatic, this one is more on general side to have discussion.
Both buildtime and runtime async component looks ok and I think is expected to work, but there are some issues with typing (both react side - should be improved in latest ts and types/react and mxd side). For now only solution seems to expect ts error.
Relevant issues: mdx-js/mdx#2242 , DefinitelyTyped/DefinitelyTyped#65003

@vercel
Copy link

vercel bot commented Jun 28, 2023

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

Name Status Preview Updated (UTC)
mailingui ✅ Ready (Inspect) Visit Preview Jun 28, 2023 0:03am

@@ -57,6 +57,7 @@ export default async function ComponentPage({

const mdxDoc = await getInstallationDoc({
componentType: type,
// @ts-expect-error async component
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tbh, I'm not sure if adding @ts-expect-error is something we'd want in our project, but I feel like I can't really make that decision on my own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I am not sure either. Just to expand I would be ok with that if I feel that its very likely this feature gonna be supported soon. But I am not really confident about that
Also thats way I extracted this into separate proposal. Maybe we will switch to @next/mdx and this wont be relevant anymore ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm looking into @next/mdx and we will see if that is a good option for us. I'm still not sure if it would be good for us.

@ericvalcik
Copy link
Collaborator

As I wrote above, I'm not sure if adding support for async components is worth adding @ts-expect-error directives into our code. IMO, I'd rather not to, but if you feel like it is a good addition I'm not against it.

Maybe @iamhectorsosa would have a better opinion on this, but he is away until next week.

@ChcJohnie ChcJohnie mentioned this pull request Jul 4, 2023
@ChcJohnie
Copy link
Collaborator Author

This PR is not relevant anymore if #130 is merged with @next-mdx lib

Base automatically changed from docs-installation to main July 11, 2023 09:34
@iamhectorsosa
Copy link
Collaborator

@ChcJohnie @ericvalcik if we need to update this PR with close with comment, please do so.

@ericvalcik
Copy link
Collaborator

I'd close this PR. @ChcJohnie if you agree, you can close this PR

@ChcJohnie
Copy link
Collaborator Author

Deprecated as next-remote-mdx is being dropped and this PR is not relevant to @next/mdx

@ChcJohnie ChcJohnie closed this Jul 12, 2023
@iamhectorsosa iamhectorsosa deleted the docs-installation-async branch July 21, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants