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

Custom markdoc tag to include markdoc file does not render react component #10418

Closed
1 task done
taktran opened this issue Mar 13, 2024 · 13 comments · Fixed by #10649
Closed
1 task done

Custom markdoc tag to include markdoc file does not render react component #10418

taktran opened this issue Mar 13, 2024 · 13 comments · Fixed by #10649
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: markdoc Related to the `@astrojs/markdoc` package (scope)

Comments

@taktran
Copy link
Contributor

taktran commented Mar 13, 2024

Astro Info

Astro                    v4.5.2
Node                     v18.18.0
System                   Linux (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             @astrojs/markdoc
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

I'm trying to create a custom markdoc tag to include other markdoc files, however it doesn't seem to work when those included files use markdoc tags that are rendered using react.

I've implemented my include-markdoc custom tag using a transform function, based on how markdoc implements partials (docs).

It does work with included files that use markdoc tags that are rendered using astro though.

The error I get is:

  TypeError: Cannot read properties of undefined (reading 'toString')
    at Object.check (/home/projects/github-mkxshb/node_modules/@astrojs/react/server.js:29:32)
    at renderFrameworkComponent (/home/projects/github-mkxshb/node_modules/astro/dist/runtime/server/render/component.js:112:33)
    at async Module.renderComponent (/home/projects/github-mkxshb/node_modules/astro/dist/runtime/server/render/component.js:374:10)
    at async AstroComponentInstance.render (/home/projects/github-mkxshb/node_modules/astro/dist/runtime/server/render/astro/instance.js:44:7)
    at async Object.render (/home/projects/github-mkxshb/node_modules/astro/dist/runtime/server/render/component.js:356:7)
    at async renderChild (/home/projects/github-mkxshb/node_modules/astro/dist/runtime/server/render/any.js:36:5)

Which points to https://github.com/withastro/astro/blob/main/packages/integrations/react/server.js#L22
It seems like a check function to see whether the node passed in is a legitimate React component or not. From console logging, the `Component passed in is something like

{
  type: 'local',
  path: './src/components/alert/Warning',
  namedExport: undefined,
  [Symbol(@astrojs/markdoc/component-config)]: true
}

So my issue is, how would I pass in a react renderer that @astro/react understands? Is there some exposed function somewhere I can use to transform the object above into a valid React component?

A side issue is that, maybe the check function needs to accommodate for this situation and not throw an error? It currently crashes the dev server and fails to build.

What's the expected result?

I can write a custom markdoc tag to include other markdoc files which contain React rendered components.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-mkxshb-tsleyv

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Mar 13, 2024
@taktran
Copy link
Contributor Author

taktran commented Mar 13, 2024

I've got it kind of working by having a wrapper Astro component: https://stackblitz.com/edit/github-mkxshb-fkgde9?file=markdoc.config.mjs%3AL22

However, the pure astro aside seems to break when used together with the react one: https://stackblitz.com/edit/github-mkxshb-l8dfld?file=src%2Fcontent%2Fdocs%2Fintro.mdoc%3AL13

with the same error

TypeError: Cannot read properties of undefined (reading 'toString')
    at Object.check (/home/projects/github-mkxshb-fkgde9/node_modules/@astrojs/react/server.js:29:32)
    at renderFrameworkComponent (/home/projects/github-mkxshb-fkgde9/node_modules/astro/dist/runtime/server/render/component.js:112:33)
    at async Module.renderComponent (/home/projects/github-mkxshb-fkgde9/node_modules/astro/dist/runtime/server/render/component.js:374:10)
    at async AstroComponentInstance.render (/home/projects/github-mkxshb-fkgde9/node_modules/astro/dist/runtime/server/render/astro/instance.js:44:7)
    at async Object.render (/home/projects/github-mkxshb-fkgde9/node_modules/astro/dist/runtime/server/render/component.js:356:7)
    at async renderChild (/home/projects/github-mkxshb-fkgde9/node_modules/astro/dist/runtime/server/render/any.js:36:5)

@taktran
Copy link
Contributor Author

taktran commented Mar 13, 2024

Oh, I got it working here: https://stackblitz.com/edit/github-mkxshb-l3fnmn?file=src%2Fcontent%2Fdocs%2Fintro.mdoc%3AL7-L9

The trick is to load the component on the page first, then it can be included in a partial.

Bug shows itself if you comment out the component on the page and the partial doesn't know about how to render the component:
https://stackblitz.com/edit/github-mkxshb-wfn81w?file=src%2Fcontent%2Fdocs%2Fintro.mdoc%3AL7-L9

It seems like there's a cache of the custom tag components loaded in markdoc.config.mjs - how would I preload those components so that the markdoc integration knows how to load it during render when including another markdoc file?

@taktran
Copy link
Contributor Author

taktran commented Mar 13, 2024

NOTE: It doesn't actually matter if the component is wrapped in an Astro component or not. If astro can render it on a page, then the included partial can render it (if it is preloaded on the page).

Also on the dev server, once it has been loaded on a page, the server knows how to render the component, and deleting it from the page still allows the included partial to render it. This also means that during builds, as long as there is one page that loads the component, then all partials can load it. If not, the build fails with the above toString error.

@ematipico ematipico added the pkg: markdoc Related to the `@astrojs/markdoc` package (scope) label Mar 15, 2024
@ematipico
Copy link
Member

@bholmesdev what do you think?

@taktran
Copy link
Contributor Author

taktran commented Mar 21, 2024

I've tried a bunch of things, and I've settled on a hack that kinda works.

  1. I have a dummy page (markdoc-custom-tags.astro) that has all the custom tags in a markdoc file that cause the breakage (ie, ones that are used in the included markdoc files, but aren't in the parent markdoc file)
  2. When my include-markdoc custom tag detects Symbol.for('@astrojs/markdoc/component-config') exists in the render tree nodes, it means it's not renderable, and throws an error
  3. On the page, I catch the error when I run await page.render(), and then redirect to the dummy page with a reference to the current page
  4. On the dummy page, I redirect back to the original page, and the missing renderable nodes should work

Here is a stackblitz with the redirect change - https://stackblitz.com/edit/github-mkxshb-5aaguk?file=README.md - however, there is an issue with the styles of the aside not showing, which doesn't happen on my project.
To fix the styling issue, I've added rendered the dummy custom tags page on the page and hidden it - https://stackblitz.com/edit/github-mkxshb-hji2gc?file=README.md

Considering it's only a dev issue, this hack is ok for now, until there is a way for me to use the astro runtime to render out the unknown nodes during a page render. Ideally, there would be an Astro API that would allow me to convert any instances of Symbol.for('@astrojs/markdoc/component-config') found in the render tree, into something that could render. Or maybe someone from the team can think of a better solution for this?

@bholmesdev
Copy link
Contributor

@taktran Oh boy, that's quite the clever solution! Sorry you have to jump through those redirect hoops but it's good to know the breakage is dev-only.

Let me know if I'm understanding your issues correctly:

  • You need a wrapper Astro component to use islands as Markdoc tags: Yes, and this is expected for tracing component info! You noticed that React encoding issue without the Astro wrapper (could use a better error here). I would like to escape the need for a wrapper component somehow, but it's unclear how the client: directive could be respected.
  • You can't use a Markdoc tag in a nested partial (top-level works fine): This is definitely a bug we'll need to fix. This could be a failure on our part to trace partials correctly to bundle components.

@taktran
Copy link
Contributor Author

taktran commented Mar 21, 2024

Hi @bholmesdev , thanks for the prompt reply!

You need a wrapper Astro component to use islands as Markdoc tags

That's not really an issue, which is a bit divergent from my original post. From this comment, I found that the issue happens with plain Astro components and React components. Having a wrapper Astro component or not, doesn't make a difference

Here is an example of the same situation happening with just react components: https://stackblitz.com/edit/github-mkxshb-vsepmb

You can't use a Markdoc tag in a nested partial (top-level works fine)

There's a bit of nuance here. You can't use a Markdoc tag in a nested partial if that Markdoc tag is a rendered component (Astro or React or presumably any other framework component) and has not been loaded in the Astro dev server runtime before. To load the Markdoc tag in the dev server runtime, you can either load it on the top level page before you include the partial, or go to a page that has the component loaded already, and then go back to the problematic page (my redirect hack). Once the Astro dev server runtime has been primed, the problematic page is no longer a problem, and can render as normal.

If you have a fresh dev server and go to the problematic page first, the dev server errors with the TypeError: Cannot read properties of undefined (reading 'toString') error from the original post.

So essentially:

  • The Astro dev server knows how to render custom markdoc components (Astro/React etc) on the top level page (expected)

  • When including markdoc as a partial, and the partial contains a custom markdoc component, if the Astro dev server has not already encountered that component, it fails to render. It fails on two levels

    On this line:

    return Component['$$typeof'].toString().slice('Symbol('.length).startsWith('react');

    Component['$$typeof'] is undefined, and throws the TypeError: Cannot read properties of undefined (reading 'toString'). However, even if that is fixed with Component['$$typeof']?.toString(), it goes through the Astro parser and fails to render. The error you get is Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object. The actual object looks like

    {
      '$$mdtype': 'Tag',
      name: {
        type: 'local',
        path: './src/components/ReactAside.astro',
        namedExport: undefined,
        [Symbol(@astrojs/markdoc/component-config)]: true
      },
      attributes: {},
      children: [
        Tag {
          '$$mdtype': 'Tag',
          name: 'p',
          attributes: {},
          children: [Array]
        }
      ]
    }
    

    This is the point I get stuck. Somehow, I'd need to be able to convert the above object into a react RenderableTreeNode, and presumably Astro has some method somewhere that can do that. Although, I'm guess there also must be some way to inject all the context that the runtime has when rendering the page.


One idea is to inject a markdocParse method into the transform config parameter, when specifying the custom tag ie,

export const includeMarkdoc: Schema = {
  // ...
  transform(node: Node, config: Config) {
    // ...
    const contents = fs.readFileSync(markdocFilePath).toString();
    const markdocAst = config.markdocParse(contents);
    // ...
  }
}

The markdocParse method needs to have all the context of how to render all the custom tags specified in markdoc.config.ts loaded at runtime, so it'd be a matter of Astro passing the internal function it uses to the transform function. Not sure how feasible this is, or whether it's the right approach though 😅

@bholmesdev
Copy link
Contributor

Thanks for the thorough explanation @taktran. I'll find some time to look at the Markdoc internals to see why partials are missed by the bundler. I know we have some pre-scanning in the dev server to hoist scripts and styles, so the partial could trip us up. The idea to add a markdocParse() function to the transform function is interesting. Is this something users would need to use when extending their Markdoc config? Or is this an internal detail?

@bholmesdev
Copy link
Contributor

bholmesdev commented Apr 1, 2024

Alright, I'm taking a look at your includeMarkdoc tag now @taktran. I'll confess I thought you were using the native Markdoc partial tag when I first read this, but I know see you are implementing your own with readFile(). I wouldn't expect this manual approach to work, since Vite would not be able to scan the file and resolve component modules appropriately (unless we added the parseMarkdoc lifecycle function as you described).

However, I'll admit you get the same issue using fs.readFile() to configure partials from your configuration file as well. I think the real bug here is "no guidance on creating Markdoc partials." We can work on this!

@bholmesdev bholmesdev added - P2: has workaround Bug, but has workaround (priority) - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged - P2: has workaround Bug, but has workaround (priority) labels Apr 1, 2024
@bholmesdev
Copy link
Contributor

Alright, I opened a PR to resolve partials internally to avoid the need for a custom includeMarkdoc tag. No partials configuration required! Use a relative file path from your markdoc file to the partial you want to render. I've also added unit tests to ensure React components are resolved correctly:

<!--src/content/blog/post.mdoc-->

{% partial file="./_diagram.mdoc" /%}

<!--src/content/blog/_diagram.mdoc-->

## Diagram

{% diagramComponent /%}

While implementing, I found we need to handle partial rendering at build time instead of creating a transform as you've built here. The PR implementation ensures validation errors point to the right file path, and components styles and scripts are discovered at build-time for React to mount correctly. This means you can't build your own partial tag in userland if you want components to render correctly. Let me know if this is a good compromise for your use case

@taktran
Copy link
Contributor Author

taktran commented Apr 2, 2024

Oh wow @bholmesdev , that sounds amazing! 😍

The includeMarkdoc was just my hack for that functionality, so being supported natively by the {% partial file="...." /%} markdoc tag is above and beyond what I thought would be possible. Validation errors are are neat bonus too!

This means you can't build your own partial tag in userland if you want components to render correctly.

I don't think I would need to build my own partial tag given your solution, so I think the compromise works fine.


Thank a bunch for the work! 🌟

@bholmesdev bholmesdev self-assigned this Apr 2, 2024
taktran added a commit to ag-grid/ag-grid that referenced this issue May 7, 2024
Use native astro markdoc support for `partial` instead.

See withastro/astro#10418 and withastro/astro#10649
taktran added a commit to ag-grid/ag-grid that referenced this issue May 7, 2024
Use native astro markdoc support for `partial` instead.

See withastro/astro#10418 and withastro/astro#10649
@taktran
Copy link
Contributor Author

taktran commented May 7, 2024

Just a follow up on this - after our upgrade to the new astro markdoc integration, I was able to successfully replace my includeMarkdoc custom tag and just use partial. I did have to write a custom function to resolve partials to get the headings within them, but that wasn't too bad.

Thanks again for the update @bholmesdev ! ❤️ So satisfying being able to delete code ✨ 😄

@bholmesdev
Copy link
Contributor

Woo, so happy to hear @taktran! Reach out with issues anytime. Want to see more Markdoc power users out there 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: markdoc Related to the `@astrojs/markdoc` package (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants