simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
141 stars 39 forks source link

Update external dependencies #86

Closed tvdijen closed 10 months ago

tvdijen commented 11 months ago

Needs testing, but the changelogs give no reason to expect any issues

tvdijen commented 11 months ago

Hmm, I just realized I've seen this same issue elsewhere ... Seems they have changed some defaults between 11.3 and 11.8 or something

khlr commented 11 months ago

What's a bit strange: I can't reproduce the unwanted behaviour out of SAML-tracer. Have a look at this fiddle: https://jsfiddle.net/fkwb3p8e/1/

There's a token string assigned to a div's textContent-property. It's done pretty much the same way as SAML-tracer does it. But in the fiddle the whole token gets highlighted correctly...

Any ideas?

tvdijen commented 10 months ago

The only possible explanation I found is in the highlight.js docs:

We strongly recommend <pre><code> wrapping for code blocks. It's quite semantic and "just works" out of the box with zero fiddling. It is possible to use other HTML elements (or combos), but you may need to pay special attention to preserving linebreaks.

I think maybe that's that we're seeing in your screenshot..

khlr commented 10 months ago

I saw that recommendation, too. But I'm not sure it "applies" to us. SAML-tracer has always represented the highlighting token this way and never used a <code> tag Sure, the argument "we have always done it this way" is not exactly the best now 😁 But I just wonder why it should cause problems right now.

And the hint only talks about line breaks... 🤔

tvdijen commented 10 months ago

I guess the highlight package has changed over time too... We have the same problem in SimpleSAMLphp where we use highlight.js to highlight metadata XML.. The entire color scheme has changed during the upgrade, but nothing in the changelog that could possibly explain this.

khlr commented 10 months ago

I took the liberty to update hljs to version 11.9.0. Whatever was changed with this version... The highlighting works now in any case again as desired...

@tvdijen: Would you please have a look, too?

khlr commented 10 months ago

Hmmm... I just took a look at the diff in our repo from version 11.8.0 to 11.9.0: Search for the string registerLanguage(". Did you just include the JavaScript-package? When I downloaded hljs today, I included HTTP and XML. Those two packages are required for the highlighting in SAML-tracer. Maybe this was just the cause for the incorrect highlighting, as hljs tried to highlight the token as JavaScript?

tvdijen commented 10 months ago

I just googled to get an updated version of the highlight.min.js file.. I was not aware of any additional packages. How did you update to 11.9?

tvdijen commented 10 months ago

Ah, I think I understand where I went wrong :') Good to know this, because I was going to create a workflow for auto-updating our dependencies, but this means I have to build a custom highlight.min.js package

khlr commented 10 months ago

Ah ok :-)

Here you can define the included packages. That's where I picked XML and HTTP. I then downloaded the ZIP file and took the highlight.min.js file.

khlr commented 10 months ago

I was going to create a workflow for auto-updating our dependencies

Very interesting! How are you going to achieve this? Using GitHub actions or something?

tvdijen commented 10 months ago

Let me rephrase that:

My goal was to migrate the build-script to actions: https://github.com/simplesamlphp/SAML-tracer/compare/main...feature/autobuild

Now, this will work because the NPM-package for Highlight.js includes all 190-something languages. I could slightly improve this by building my own highlight.min.js file based on highlight.js/core + the two language packs, but since this is an extension and not a package transfered over the line, the added value would be minimal.

khlr commented 10 months ago

That sounds really promising, Tim!

However I would suggest that we should discuss this in another issue resp. alongside another PR's discussion, do you agree?

Do you agree then in merging this PR here?