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

fix: patch lodash to not use Function calls #5408

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

Yash-Singh1
Copy link
Member

@Yash-Singh1 Yash-Singh1 commented Mar 23, 2024

📑 Summary

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

Resolves #5378

📏 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.

📋 Tasks

Make sure you

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Mar 23, 2024
Copy link

netlify bot commented Mar 23, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 9fd6a13
🔍 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.

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

Impacted file tree graph

@@           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 Coverage Δ
unit 5.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@Yash-Singh1 Yash-Singh1 marked this pull request as draft March 23, 2024 19:55
@Yash-Singh1
Copy link
Member Author

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
Copy link

simov commented Mar 24, 2024

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

@Yash-Singh1
Copy link
Member Author

@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
Copy link

simov commented Mar 24, 2024

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 #5383 about the character encoding seems to be resolved too.

@Yash-Singh1 Yash-Singh1 marked this pull request as ready for review March 25, 2024 00:02
@Yash-Singh1
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mermaid versions after v9.2.2 cannot be injected into Firefox addons
3 participants