search

mermaid-js / mermaid

Generation of diagrams like flowcharts or sequence diagrams from text in a similar manner as markdown
https://mermaid.js.org
MIT License
68.79k stars 6.08k forks source link

fix: patch lodash to not use `Function` calls #5408

Open Yash-Singh1 opened 3 months ago

Yash-Singh1 commented 3 months ago

:bookmark_tabs: Summary

Patch lodash-es and cytoscape (has lodash bundled in it) to avoid using Function calls.

Resolves #5378

:straight_ruler: Design Decisions

I patched lodash-es to use a globalThis polyfill and for cytoscape I just used window because cytoscape only runs in browser afaik.

This won't fix CSP violations when running Mermaid.js without the bundles (directly importing the ESM mermaid.core.mjs), but it would patch the Function calls in the bundles.

:clipboard: Tasks

Make sure you

netlify[bot] commented 3 months ago

Deploy Preview for mermaid-js ready!

Name Link
Latest commit 9fd6a1396b0b3cd666cf053a4dd3ecf6a679779e
Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65ff34f98009b80008c5b593
Deploy Preview https://deploy-preview-5408--mermaid-js.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 5.72%. Comparing base (c00bf26) to head (9fd6a13).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/mermaid-js/mermaid/pull/5408/graphs/tree.svg?width=650&height=150&src=pr&token=BaET4V1BdM&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mermaid-js)](https://app.codecov.io/gh/mermaid-js/mermaid/pull/5408?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mermaid-js) ```diff @@ Coverage Diff @@ ## develop #5408 +/- ## ======================================= Coverage 5.72% 5.72% ======================================= Files 277 277 Lines 41896 41896 Branches 27 27 ======================================= Hits 2400 2400 Misses 39495 39495 Partials 1 1 ``` | [Flag](https://app.codecov.io/gh/mermaid-js/mermaid/pull/5408/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mermaid-js) | Coverage Ξ” | | |---|---|---| | [unit](https://app.codecov.io/gh/mermaid-js/mermaid/pull/5408/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mermaid-js) | `5.72% <ΓΈ> (ΓΈ)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mermaid-js#carryforward-flags-in-the-pull-request-comment) to find out more.
Yash-Singh1 commented 3 months ago

I got this working with the example repro provided by @simov on Firefox 124.

@simov Could you confirm that this works on your end too?

simov commented 3 months ago

@Yash-Singh1 sure, can you point me to the mermaid.min.js produced by this PR?

Yash-Singh1 commented 3 months ago

@Yash-Singh1 sure, can you point me to the mermaid.min.js produced by this PR?

Sure, here is a link to the mermaid.min.js build: https://mermaid-upload-5408-build.pages.dev/9fd6a13/mermaid.min.js

LMK if you need anything else to get it working.

simov commented 3 months ago

It works in Firefox.

Is this PR on top of v10.9.0? Because it works in Chrome too, meaning the other issue reported here https://github.com/mermaid-js/mermaid/issues/5383 about the character encoding seems to be resolved too.

Yash-Singh1 commented 3 months ago

Is this PR on top of v10.9.0?

Yes, this PR is on top of v10.9.0.

Because it works in Chrome too, meaning the other issue reported here #5383 about the character encoding seems to be resolved too.

Interesting, there must have been a commit between now and the 10.9.0 release that removed the latin characters issue then.

sidharthv96 commented 3 months ago

Nice work @Yash-Singh1! What do you think the long term fix for this would be? Should we raise the issue in lodash repo to see if they would add the polyfill directly?

Yash-Singh1 commented 3 months ago

Nice work @Yash-Singh1! What do you think the long term fix for this would be? Should we raise the issue in lodash repo to see if they would add the polyfill directly?

Yeah, I think that would make sense.