-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
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 |
@stevejpurves and I have overhauled this and it will be released shortly in @GYHHAHA let me know if you need any help upgrading anything! |
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. Thechildren
would also now be on the react component, and not inside of thenode.children
.This issue was raised in #95, which is a bit confusing why this doesn't work.
The text was updated successfully, but these errors were encountered: