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

Improve head tag parsing on template fragments #2024

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

fercomunello
Copy link
Contributor

@fercomunello fercomunello commented Nov 21, 2023

Description

This PR:

  • Fix the duplication of head's children tags inside the content when parsing fragments regardless of the status of useTemplateFragments configuration.
  • These modifications are applied for both parsers: template fragment and plain HTML (default). The new strategy is to skip head tag parsing on the makeFragment(response) function as head-support extension already implemented a proper merging algorithm for that.
  • Added a manual .html test: hxboost_partial_template_parsing/index.html, index-partial.html & other-content.html.
  • Also added an array with similar regular expressions to be compiled only once and not for every execution (edit: replaced the array approach by constants).

Corresponding issue: #2018

This PR needs a review. For knowledge: @1cg @alexpetros

Testing

  • If the template fragment is parsed incorrectly and end up including head's children tags inside the template body, the manual test won't have the expected behaviour.
  • In order to make the manual test fail just execute it without the modifications or uncomment the following line on htmx.js:
    content = content.replace(regexPatterns.HEAD, '');

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

Copy link
Collaborator

@alexpetros alexpetros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this looks great. Will try to get it slated for 1.9.10

src/htmx.js Outdated Show resolved Hide resolved
@xhaggi
Copy link
Contributor

xhaggi commented Nov 28, 2023

I think this PR would benefit from #1794. It would be much easier to identify the head element, regardless of where it appears on the top level. The current implementation of makeFragment relies heavily on the order of the elements.

@fercomunello
Copy link
Contributor Author

@xhaggi Hmm, interesting. Let's see when I've some time to playground using your PR as an entrypoint, thanks! :-)

@alexpetros
Copy link
Collaborator

alexpetros commented Dec 3, 2023

I'll comment on #1794 but I think this PR is good. The response parser is one of the more complicated things htmx does so I can't promise we'll accept it, but this change in intelligible enough that we can at least review it.

@alexpetros alexpetros added the ready for review Issues that are ready to be considered for merging label Dec 3, 2023
@1cg 1cg merged commit e9bce8d into bigskysoftware:dev Dec 20, 2023
2 checks passed
@MichaelWest22 MichaelWest22 mentioned this pull request Aug 1, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants