search

mojolicious / mojo

:sparkles: Mojolicious - Perl real-time web framework
https://mojolicious.org
Artistic License 2.0
2.66k stars 577 forks source link

Fixed: Mojo::DOM doesn't recognize end of comment #2029 #2030 #2082

Open poti1 opened 1 year ago

poti1 commented 1 year ago

Summary

Mojo::DOM doesn't recognize end of comment (when it should) #2030 Mojo::DOM treats "-- >" as end of comment (it shouldn't) #2029

Motivation

EXPLAIN WHY YOU BELIEVE THESE CHANGES ARE NECESSARY HERE

References

LIST RELEVANT ISSUES, PULL REQUESTS AND FORUM DISCUSSIONS HERE

kraih commented 1 year ago

That commit message tells me nothing.

kraih commented 1 year ago

Since both issues were reported by @mauke, a review would be appreciated.

poti1 commented 1 year ago

That commit message tells me nothing.

I am not sure how to best proceed. (Should I revert and update the comment?)

kraih commented 1 year ago

I am not sure how to best proceed. (Should I revert and update the comment?)

Just change the commit message to something meaningful.

kraih commented 1 year ago

You changed the title of the PR, not the commit message. I'll mark this PR as work in progress for now, so nobody reviews it yet by accident.

poti1 commented 1 year ago

You changed the title of the PR, not the commit message. I'll mark this PR as work in progress for now, so nobody reviews it yet by accident.

Not sure how to just change the commit message here without submitting another commit.

rawleyfowler commented 1 year ago

@poti1 

git commit --amend -m <useful commit message>
git push --force <origin> <branch>
poti1 commented 1 year ago

I didn't know about the --ammend option. Thanks

poti1 commented 1 year ago

@kraih Anything missing from my side?

kraih commented 1 year ago

Wonder if those lookahead assertions will end up being a performance issue in the future when someone finds a weird enough HTML document.

poti1 commented 1 year ago

I completely forgot what the fix even was now 😅

poti1 commented 1 year ago

Wonder if those lookahead assertions will end up being a performance issue in the future when someone finds a weird enough HTML document.

Any suggestions?

poti1 commented 1 year ago

Maybe we can use an "unrolling-the-loop" pattern 🤔

kraih commented 1 year ago

Something that's also easy to port to the JavaScript version would be neat. 😁

poti1 commented 1 year ago

I know Mojo::DOM. What's the JavaScript version? (I'm still not familiar with all Mojo features)

kraih commented 1 year ago

I know Mojo::DOM. What's the JavaScript version? (I'm still not familiar with all Mojo features)

https://github.com/mojolicious/dom.js