SemanticMediaWiki / Mermaid

Provides a parser function to generate diagrams and flowcharts with the help of the mermaid script language
https://www.mediawiki.org/wiki/Extension:Mermaid
Other
36 stars 23 forks source link

Update to 10.6.1 #102

Open sneakers-the-rat opened 9 months ago

sneakers-the-rat commented 9 months ago

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:

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:

sneakers-the-rat commented 9 months ago

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)

ProbablePrime commented 7 months ago

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 commented 7 months ago

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

Thanks!

sneakers-the-rat commented 7 months ago

heck yes thank you for fixing this :)

ProbablePrime commented 7 months ago

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

ProbablePrime commented 7 months ago

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

krabina commented 7 months ago

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

ProbablePrime commented 7 months ago

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:

sneakers-the-rat commented 7 months ago

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

ProbablePrime commented 7 months ago

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

gesinn-it-gea commented 7 months ago

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 commented 7 months ago

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

sneakers-the-rat commented 7 months ago

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

gesinn-it-gea commented 7 months ago

see fixes introduced by #104. Maybe rebase.

JeroenDeDauw commented 2 months ago

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

sneakers-the-rat commented 2 months ago

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