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

centralize md instance as part of the normalized configuration #1034

Merged
merged 5 commits into from Mar 11, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 10, 2024

A bit of refactoring before addressing #92, and maybe reviving #702.

@Fil Fil requested a review from mbostock March 11, 2024 09:16
@Fil Fil marked this pull request as ready for review March 11, 2024 09:16
@Fil Fil requested a review from cinxmo March 11, 2024 09:16
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

It’d be helpful to have some more context in the PR description as to why we want to do this. It seems mostly internally-focused and the advantage of doing this isn’t clear to me.

Also, one particular risk I noticed is that the md argument is optional to parseMarkdown, meaning that it will fallback to a default instance if we forget to pass it in. This pattern can sometimes mask bugs because there are multiple call points to this function, and in particular we parse again during live update and that codepath is not well tested by unit tests.

And lastly there is #597 which may want to remove our dependency on markdown-it entirely… although I’m guessing we’ll instead want to do inline expressions as a preprocessing step and keep markdown-it.

Anyway I would like at least to understand the goal here so we can prioritize.

@mbostock mbostock removed the request for review from cinxmo March 11, 2024 13:32
@Fil
Copy link
Contributor Author

Fil commented Mar 11, 2024

As I was working on #92 (now with PR #1037) I wanted to pass down an additional option to markdown ("cleanUrls"). I saw that we were creating a new instance of md for each page, with a root option that wasn't used at all, and a path option that was redundant (it could be read from the context). We were also calling markdownIt — the new option that adds custom plugins — once per page. All this seemed non-optimal on principle, and I didn't want to add another option on top without cleaning this up.

Also, as suggested by the link to #702, this could allow an implementation of md that makes use of a cache, to avoid parsing repeatedly the same files when we build.

The optional md is there for some of the unit tests, I can give them a custom md instead to avoid hiding a bug.

Creating a new instance of md is fast, so this did not translate into visible gains in execution time.

@Fil Fil mentioned this pull request Mar 11, 2024
@mbostock mbostock enabled auto-merge (squash) March 11, 2024 23:04
@mbostock mbostock merged commit a94ce04 into main Mar 11, 2024
4 checks passed
@mbostock mbostock deleted the fil/config-md branch March 11, 2024 23:07
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.

None yet

2 participants