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

Update to 10.6.1 #102

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link

Surpasses #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
Copy link
Author

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

ProbablePrime commented Feb 16, 2024

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

sneakers-the-rat#1 now contains the updates required to address the issues discussed here.

Thanks!

@sneakers-the-rat
Copy link
Author

heck yes thank you for fixing this :)

@ProbablePrime
Copy link

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

@ProbablePrime
Copy link

ProbablePrime commented Feb 16, 2024

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: Yellow-Dog-Man@22da6eb

There's another fix here too: Yellow-Dog-Man@eb1bbfb

@krabina
Copy link
Contributor

krabina commented Feb 16, 2024

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

@ProbablePrime
Copy link

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

@sneakers-the-rat
Copy link
Author

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

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

@gesinn-it-gea
Copy link
Member

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

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

@sneakers-the-rat
Copy link
Author

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

see fixes introduced by #104. Maybe rebase.

@JeroenDeDauw
Copy link
Member

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

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

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.

5 participants