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

Refactor NodeRenderer interface #97

Closed
rowanc1 opened this issue May 27, 2023 · 2 comments
Closed

Refactor NodeRenderer interface #97

rowanc1 opened this issue May 27, 2023 · 2 comments

Comments

@rowanc1
Copy link
Collaborator

rowanc1 commented May 27, 2023

The current node renderer is not a react function, which can lead to problems if you use hooks inside of them. I think that we should be able to refactor this without too much effort, and it will be much simpler and lead to fewer bugs.

This will be a breaking change, but I don't think too many folks are extending at this point, so it is a good idea to do it now rather than later. @GYHHAHA are you currently extending the renderers?

I think the change would be changing the function signature from MyRenderer(node) --> MyRenderer({ node, children }) and then changing where we integrate this in the code. That change allows these to actually be react nodes. The children would also now be on the react component, and not inside of the node.children.

This issue was raised in #95, which is a bit confusing why this doesn't work.

@GYHHAHA
Copy link
Contributor

GYHHAHA commented May 27, 2023

I indeed am extending customized renderers, but they works fine. The problem appears when using the OUTPUT_RENDERERS themselves, which I didn't make any change on that, directly import from jupyter pkg. Here is an example show the way I use it, (another thing is I have not used any theme also)

export function parse(mdast: any) {
  const linkTransforms = [
    new WikiTransformer(),
    new GithubTransformer(),
    new DOITransformer(),
    new RRIDTransformer(),
  ];

  unified()
    .use(basicTransformationsPlugin)
    .use(mathPlugin)
    // .use(enumerateTargetsPlugin, { state })
    .use(linksPlugin, { transformers: linkTransforms })
    .use(footnotesPlugin)
    // .use(resolveReferencesPlugin, { state })
    .use(keysPlugin)
    .runSync(mdast as any);

  const content = useParse(mdast, {
    ...DEFAULT_RENDERERS,
    ...OUTPUT_RENDERERS,
    ...CUSTOMIZED_RENDERERS
  });
  return { content };
}

I'm fine with your potential changes, even if it might break some codes. However, since I'm able to lock the @myst-theme/jupyter as 0.1.36, it won't hurt too much at least for now. As long as it's fixed on your side, I'll immediately launch a another round test for that. Thanks! @rowanc1

@rowanc1
Copy link
Collaborator Author

rowanc1 commented Jul 17, 2023

@stevejpurves and I have overhauled this and it will be released shortly in v0.4.0. This means that all of the NodeRenderers are just react components, and must render their own children using the <MyST ast={node} /> component. This allows components to have control over their own children, improves incremental rendering and is a much simpler interface!

@GYHHAHA let me know if you need any help upgrading anything!

@rowanc1 rowanc1 closed this as completed Jul 17, 2023
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

No branches or pull requests

2 participants