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

Fixed so PrismJS and Mermaid can be used together #318

Merged
merged 12 commits into from Aug 3, 2022

Conversation

applejag
Copy link
Contributor

@applejag applejag commented Jul 23, 2022

Fixes PrismJS script to work with Mermaid.

Had to use manual syntax highlighting, and in there filter out the mermaid tags. Also had to do some "is hot-reload" checking to make it more reliable when using emanote run

I took inspiration from PrismJS/prism#2068 (comment), which the filter-highlight-all PrismJS plugin was created to simplify anyway.

Also changed some docs:

  • Make Mermaid page be structured the same as the Syntax Highlighting page, and duplicated the diagrams so the docs shows the source code as well.
  • Changed to only suggest <script var="js.prism" /> for Syntax Highlighting

Related to discussion #281

Sidenote

I tried using https://prismjs.com/plugins/filter-highlight-all/, which worked fine until the hot-reload kicked in.

My theory is that while running emanote run and editing a file that the hot-reload would recreate the <head> element and thereby recreate the PrismJS script tag, making it run again.

In the happy path when PrismJS is loaded in first time, it waits until the document has finished loading, and also letting PrismJS plugins to load as well.

But when it ran again after a hot-reload then the document was already ready and it started syntax highlighting before the filter-highlighting-all plugin could register its exceptions, so it still would apply syntax highlighting on those poor <code class="mermaid"> tags.

@applejag applejag marked this pull request as draft July 23, 2022 19:33
@applejag applejag marked this pull request as ready for review July 23, 2022 20:08
Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

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

Thanks; this does solve the original problem. However, the code needs comments explaining what it does in various places.

default/index.yaml Show resolved Hide resolved
Comment on lines 100 to 107
window.isHotReload = window.isHotReload !== undefined;
if (window.isHotReload) {
highlightAllExceptMermaid();
} else {
window.addEventListener('load', function() {
highlightAllExceptMermaid();
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a comment explaining what window.isHotReload does?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm now less sure of what this is achieving When I tried bin/run locally, the old problem (of syntax highlighting not being applied in some cases when switching routes, in live server) remains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I've finally found the cause here. The morphdom from Ema is removing the <script async src=".../mermaid.js"> tags.

When the highlightElement is called, the autoloader plugin adds an async script tag to the <body> and sets a callback on its onload.

But here a race condition comes in. I'm only guessing here, but it seems like either:

  • (happy path) the <script> is loaded and onload is called before it's removed from <body>,
  • or the <script> is removed before the browser even starts loading the script resulting in no outcomes (no errors logs, and no syntax highlighting),
  • or the browser starts fetching the JS file and at the point the fetch completes the <script> tag has already been removed resulting in code error as the autoloader plugin tries to remove the <script> tag itself when it no longer exists.

I've tried to combat this, and the code grew. Not my proudest work at all. What do you think?

It's a little glitchy when first opening a page, but it at least resolves into a syntax highlighted page eventually. And there's no glitchyness after a hot reload or after just loading the statically generated site.

Copy link
Owner

@srid srid Jul 28, 2022

Choose a reason for hiding this comment

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

Do you think this whole debounce logic is worth upstreaming to Ema (its multisite branch)? cf. srid/ema#69

ie., do we envision doing this for anything other than PrismJS?

Copy link
Owner

Choose a reason for hiding this comment

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

In particular, perhaps we can abstract this out in a function (in Ema) and call it in one line as:

Ema.invoke(1000, highlightAllExceptMermaid);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a debounce function to upstream Ema library just feels like adding it for the sake of it. I'd vote for no as it's quite the simple function.

Anyway that decision is not written in stone. Could extract it first when a need for it appears in multiple places.

Besides, this code is actually me working around Ema's behavior, but perhaps that could be modified instead. Like when using emanote run then on a fresh page load it will:

  1. run scripts (on regular page load)
  2. run scripts when websocket connected
  3. load page again (via websockets) and morph DOM
  4. run script after DOM morph

I see you have comments in the JavaScript in the sorts of "We have to reload <script>'s here on initial page load here, so as to make Twind continue to function", and I don't know the full depth of this, but I think the best option would be if scripts only ran once and morphing the DOM only happens on page changes and not on initial load.

docs/tips/js/syntax-highlighting.md Outdated Show resolved Hide resolved
@applejag applejag requested a review from srid July 25, 2022 20:00
@srid
Copy link
Owner

srid commented Aug 1, 2022

@jilleJr Could you move the debounce stuff to a separate PR since that appears unrelated? Would be easier to review and merge.

EDIT: you can make it on top of this PR's branch to avoid conflicts.

@applejag
Copy link
Contributor Author

applejag commented Aug 1, 2022

@jilleJr Could you move the debounce stuff to a separate PR since that appears unrelated? Would be easier to review and merge.

EDIT: you can make it on top of this PR's branch to avoid conflicts.

Ehm sure. The debounce logic is a requirement to get this working during emanote run though, but if I get you right then I'll simplify this one and only focus on emanote gen here

@srid srid merged commit 8bd7cf6 into srid:master Aug 3, 2022
@applejag applejag deleted the feature/prism-x-mermaid branch August 3, 2022 18:26
srid added a commit that referenced this pull request Aug 12, 2022
srid added a commit that referenced this pull request Aug 12, 2022
Revert #318; and, use `highlight.js` as default in docs
shivaraj-bh pushed a commit to shivaraj-bh/emanote that referenced this pull request Dec 18, 2023
Co-authored-by: Sridhar Ratnakumar <srid@srid.ca>
shivaraj-bh pushed a commit to shivaraj-bh/emanote that referenced this pull request Dec 18, 2023
shivaraj-bh pushed a commit to shivaraj-bh/emanote that referenced this pull request Dec 18, 2023
Revert srid#318; and, use `highlight.js` as default in docs
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