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

Use IE conditional comment placeholders for server island start markers #12090

Conversation

markjaquith
Copy link
Contributor

@markjaquith markjaquith commented Sep 28, 2024

Changes

fixes #11995

cc: @matthewp — we talked about this on Twitter.

Swaps the generic HTML comment placeholders with IE conditional comments. These placeholders have existed for a long time, and are much less likely to be removed by HTML minifiers, because they have traditionally been used to change the behavior of a site.

A similar fix was implemented in Livewire to work around Cloudflare HTML minification removing the HTML comment placeholders Livewire was inserting for a similar purpose.

Testing

No tests changed, since it's just changing the content of an HTML comment. I haven't been able to set up the original commenter's complete environment for a complete fix verification, but I did look at the source code for the minifier library they were using, and that library has code to not remove IE conditional comments.

Docs

No documentation needed... the feature should just work more reliably now when Astro output is touched by HTML minifiers.

Copy link

changeset-bot bot commented Sep 28, 2024

🦋 Changeset detected

Latest commit: 5de434f

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 28, 2024
@markjaquith markjaquith force-pushed the issue/11995/server-island-placeholder-comment branch from 660fd38 to 079d3a4 Compare September 28, 2024 22:20
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Sep 28, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@@ -47,7 +47,7 @@ export function renderServerIsland(
}
}

destination.write('<!--server-island-start-->');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

astro here isn't anything special... it just needs to be a condition that evaluates to false. I wasn't sure about putting server-island-start in the condition part, because I was worried the dashes might break the regex minification libraries rely on to identify these conditional comments. As a bonus, it identifies "hey, Astro put this weird comment here" to anyone who might happen to see it and wonder. But this could be if false or if serverislandstart just as easily.

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me!

These placeholders are much less likely to be removed by HTML minifiers,
because they have traditionally been used to change the behavior of
site.
@markjaquith markjaquith force-pushed the issue/11995/server-island-placeholder-comment branch from b58e9c8 to 5de434f Compare September 28, 2024 22:26
@markjaquith markjaquith reopened this Sep 28, 2024
@markjaquith
Copy link
Contributor Author

I changed this from a minor to a patch change, as I regard it as being backward compatible. Someone will have to manually update the label, I guess they don't get updated if you change that.

@markjaquith markjaquith marked this pull request as ready for review September 28, 2024 22:29
@Princesseuh
Copy link
Member

The changeset here should be patch anyway, as the feature it updates is experimental on this branch.

@Princesseuh Princesseuh removed the semver: minor Change triggers a `minor` release label Oct 1, 2024
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

I can't believe the fix is this simple 😅 Thanks so much @markjaquith

@matthewp matthewp merged commit d49a537 into withastro:main Oct 1, 2024
18 of 27 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Unintended DOM Manipulation in Server Island Component Replacement
3 participants