Mermaid icon indicating copy to clipboard operation
Mermaid copied to clipboard

Update to 10.6.1

Open sneakers-the-rat opened this issue 1 year ago • 18 comments
trafficstars

Surpasses https://github.com/SemanticMediaWiki/Mermaid/pull/96

96 has been open for about a year now, let's merge this thing, 8.4.3 is old as the hills!

This PR addresses or contains:

  • Update mermaid to 10.6.1
  • Update extension config using 'scripts' to 'packageFiles' as is now recommended
  • Remove sourcemap since mermaid no longer distributes them - they're also moving to esm only in 11 i think but this should work for now
  • Update calling syntax since init is deprecated and will be removed in 11
  • Remove wrapping closure since it's no longer needed, and packageFiles don't get the document loaded event
  • Updated config fields by printing the default config and doing some string manipulation to clean it up.

I don't write PHP so no tests, but i did test a handful of diagrams and they all work fine on my instance (1.38.4)

Prior errors in 96 are mostly from not injecting /*@nomin*/ when installing by cloning from git. Since we're vendoring here, I just added it - the double minification breaks the JS by removing function names and breaking in inappropriate places.

This PR includes:

  • [ ] Tests (unit/integration)
  • [ ] CI build passed

sneakers-the-rat avatar Jan 10 '24 03:01 sneakers-the-rat

There is something funny in how it's handling URI encoding/decoding now - the JS sees > as "&gt" but mediawiki renders as a proper >, messing up mermaid diagrams with templates (basically turning directed edged into undirected edges and putting the > in the label)

sneakers-the-rat avatar Jan 11 '24 06:01 sneakers-the-rat

I'm having issues with this. Uncaught SyntaxError: Function statements require a function name (at load.php?lang=en&modules=ext.mermaid%7Cjquery&skin=citizen&version=1spug:380:1)

Which of course leads to a minified minefield of mess image

There's a line break there in the output from Mediawiki that's not present in the file. I think Mediawiki is still minifying this, which is very annoying. With the line break we get invalid JS

I also upgraded the minified file to the latest mermaid: 10.8.0. Which as similar issues: "Uncaught SyntaxError: Illegal newline after throw (at load.php?lang=en&modules=ext.mermaid%7Cjquery&skin=citizen&version=1ln6w:2434:996)"

I wrapped some curly braces around the areas that the minifier was confused about and have now hit your "&gt" issue. I'll fix that now.

This is exceptionally frustrating.

ProbablePrime avatar Feb 16 '24 04:02 ProbablePrime

https://github.com/sneakers-the-rat/SMW-Mermaid/pull/1 now contains the updates required to address the issues discussed here.

Thanks!

ProbablePrime avatar Feb 16 '24 04:02 ProbablePrime

heck yes thank you for fixing this :)

sneakers-the-rat avatar Feb 16 '24 05:02 sneakers-the-rat

Update here is that it appears some diagrams still malfunction. I'm diagnosing.

ProbablePrime avatar Feb 16 '24 06:02 ProbablePrime

I patched an error that occurs with flowcharts by manually editing the minified js, I hate that I am doing this but that commit is here: https://github.com/Yellow-Dog-Man/SMW-Mermaid/commit/22da6eb212c775f0f117bcb9112794f5121cfd67

There's another fix here too: https://github.com/Yellow-Dog-Man/SMW-Mermaid/commit/eb1bbfbf2c9feebc20ba1cc3e59ca536b2d769fb

ProbablePrime avatar Feb 16 '24 07:02 ProbablePrime

Thank you for your contribution! Tested this manually and does work for me. All of my test mermaids are working

  • https://test.knowledge.wiki/Mermaid
  • https://test.knowledge.wiki/MermaidGantt
  • https://test.knowledge.wiki/MermaidTest
  • https://test.knowledge.wiki/MermaidTimeline
  • https://test.knowledge.wiki/MermaidSankey (new since v 10) @gesinn-it-gea can we merge this?

krabina avatar Feb 16 '24 08:02 krabina

Thank you for your contribution! Tested this manually and does work for me. All of my test mermaids are working

With CI failing that's unlikely and with my minifcation issues i wouldn't recommend it.

We need to figure out:

  • Why I needed to edit the minified mermaid out?
    • Why Mediawiki appears to STILL be minifying this code a second time,
  • The tests

ProbablePrime avatar Feb 17 '24 00:02 ProbablePrime

I am thinking we should probably just be vendoring the unminified version of it, since mediawiki says "don't do this" https://www.mediawiki.org/wiki/ResourceLoader/Foreign_resources#Minified_files

at the point when there are multiple bugs that require us to manually edit the minified js (ditto @ProbablePrime i hate doing this lol) then we should probably not just try and hack our way through.

the CI failures seem like trivial strcmp differences, eg.

\" instead of "

so that part probably is just a matter of updating the tests to some new parsing

sneakers-the-rat avatar Feb 17 '24 21:02 sneakers-the-rat

I did see Foreign Resources, but those don't look compatible with extensions.

ProbablePrime avatar Feb 18 '24 02:02 ProbablePrime

Thank you for your contributions! We should take a closer look at why CI fails. There are also some warnings in CI that should be addressed. Also we should check this recent version in combination with Semantic Result Formats.

gesinn-it-gea avatar Feb 19 '24 11:02 gesinn-it-gea

@gesinn-it-evl can you have a look tomorrow, please.

gesinn-it-gea avatar Feb 19 '24 11:02 gesinn-it-gea

I did see Foreign Resources, but those don't look compatible with extensions.

Looking at them again, im pretty sure they provide a mechanism to source from NPM directly, since vendoring is not a great approach if we can avoid it. Ill try it and see

sneakers-the-rat avatar Feb 19 '24 22:02 sneakers-the-rat

see fixes introduced by #104. Maybe rebase.

gesinn-it-gea avatar Feb 26 '24 12:02 gesinn-it-gea

@sneakers-the-rat could you rebase against master? Probably the tests work now. And since @krabina already tested manually, we'd be able to merge.

JeroenDeDauw avatar Jul 28 '24 19:07 JeroenDeDauw

rebased, i admit i have sorta lost the thread on this one, reading back it seems like vendoring minified version of mermaid is just always going to have some problems, but won't have time to do any substantial work to try and understand how to do that for a bit, and could follow on with another PR after this one if it doesn't work

sneakers-the-rat avatar Jul 29 '24 00:07 sneakers-the-rat