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

Use mermaid.mermaidAPI to render #16

Closed
wants to merge 1 commit into from

Conversation

DanInProgress
Copy link

  • Adds more stable rendering
  • Adds improved error handling
  • Reuses browser page between renders
  • Updates mermaid
  • patch updates uninst-util-visit

This resolves #13 and makes rendering more predictable overall. I think the only concern I have is that there could be scaling/memory leak issues with reusing the same rendering environment. mermaidAPI appears to clean up after itself, but I figured it was worth a note here.

I did not yet update the package version number, as I figured you could take care of this post-merge. (I'm still on the fence as to whether this should be a minor or a major break)

changes to functionality

Now if someone uses escaped characters in their diagrams, they will just stay escaped. This breaks if people used the workaround in #13 and corrects it to have the intended behavior.

- Adds more stable rendering
- Adds improved error handling
- Reuses browser page between renders
- Updates mermaid
- patch updates uninst-util-visit
@DanInProgress
Copy link
Author

Oh, this probably needs a rebase now, I'll revisit this later this week (I've been using my fork)

@DanInProgress
Copy link
Author

Actually, @bdenham 's PR looks like it satisfies anything that mine could. I'm going to go ahead and close. Glad to see an update getting some traction!! :)

@remcohaszing
Copy link
Owner

Somehow I missed updates on this repository.

Anyway, that PR fixes a lot of issues indeed. But it doesn’t use mermaidAPI. Do you still think that would be an improvement? If so, why? I’m mainly interested because of remcohaszing/remark-mermaidjs#5. (That project now handles most logic of this package.)

@DanInProgress
Copy link
Author

If my memory serves, I stumbled across this issue because I noticed that the text being injected into the <pre> tags was not being escaped properly before being injected. This led to < and > being rendered incorrectly (and rendered differently than the online editor. Using mermaidApi avoids the need to do this entirely and returns the string to you directly (see this snippet from my own repo:

// page:            a puppeteer page object
// id:                 the id of an element on the page (I think this is needed so that d3 can manipulate it)
// definition:   the raw string taken from remark 
async function render(page, id, definition) {

    const {success,svgCode,error} = await page.evaluate(async (id, definition) => {
        try {
            const svgCode = await window.mermaid.mermaidAPI.render(id, definition)
            return {
                success: true,
                svgCode,
                error: null
            }
        } catch (e) {
            return {
                success: false,
                svgCode: null,
                error: JSON.stringify(e, Object.getOwnPropertyNames(e))
            }
        }
    }, id, definition);

    if(success){
        return svgCode
    } else {
        throw new HeadlessMermaidError("failed to render SVG", JSON.parse(error))
    }
}

I think this is still an improvement, but I can open a PR on that repo with the changes if you would like!

as for the instability issue you're facing, the first places I would check would be:

  • are you treating rendering responses as promises?

They changed this for some Apis in the last year, it bit me when I upgraded.

  • how diagram code is being injected?

When injection/non-compliant html is possible, I've found timing-dependent behavior

  • are you using unique element ids for each diagram?

my code does this--I don't remember why I added it, but seems like a place where problems could occur if a div is reused (I'm also noticing a bug in my code... I think I'm leaking svg elements...)

@remcohaszing
Copy link
Owner

It looks like mermaid.render is an alias of mermaid.mermaidAPI.render (https://github.com/mermaid-js/mermaid/blob/v9.2.2/packages/mermaid/src/mermaid.ts#L534).

are you treating rendering responses as promises?

Mermaid doesn’t return promises. It’s fully synchronous.

how diagram code is being injected?

It’s properly injected. remark-mermaidjs has a pretty decent test suite using snapshots. It’s just inconsistent sometimes.

are you using unique element ids for each diagram?

Yes

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.

Doesn't work with <<fork>> option
2 participants