Bug: Unintended DOM Manipulation in Server Island Component Replacement

twodft commented 1 month ago

Astro Info

Astro                    v0.0.0-si-get-20240815175345
Node                     v20.13.1
System                   Windows (x64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/node
Integrations             @astrojs/react
                         @astrojs/tailwind
                         astro-compress

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

There appears to be a potential bug in the DOM manipulation logic for replacing server island components. The current implementation may, under certain conditions, remove unintended parts of the DOM, potentially clearing the entire page content.

while(script.previousSibling &&
    script.previousSibling.nodeType !== 8 &&
    script.previousSibling.data !== 'server-island-start') {
    script.previousSibling.remove();
}
script.previousSibling?.remove();

here I found this code, it is designed to remove all sibling nodes preceding the current script element until it encounters a comment node (nodeType === 8) with the content 'server-island-start'. However, if this comment  node is missing or improperly positioned, the loop continues to remove elements, potentially clearing the entire page content.

Reproduction

Clone this repo https://github.com/twodft/server-islands-reprod build and the preview those page:

http://localhost:4321/bug/bug This page got wrong, obviously
http://localhost:4321/ This page works fine since  did not removed by astro compress
http://localhost:4321/bug/test This page also works fine, because server island component is wrapped with a parent div, <div><CustomerReviews server:defer /></div>

What's the expected result?

The astro server island script should only replace the content of its corresponding server island component, leaving the rest of the page intact.

Link to Minimal Reproducible Example

https://github.com/twodft/server-islands-reprod

Participation

[ ] I am willing to submit a pull request for this issue.

matthewp commented 1 month ago

Thanks. Haven't looked at the repro yet, but what's the thing that will cause the comment to be in the wrong position?

twodft commented 1 month ago

Thanks. Haven't looked at the repro yet, but what's the thing that will cause the comment to be in the wrong position?

I use astro-compress to minify build out static HTML files, this will remove all comments include the , this caused the issue

matthewp commented 1 month ago

Oh I see. Others have reported this sort of thing, but there's really nothing we can do about that, the comment is definitely needed. Can you tell astro-compress to allow certain comments to remain in?

matthewp commented 1 month ago

Probably the ideal thing here would be to give an error message when it can't find the comment node. I don't know how I feel about logging errors in the dev console in prod though.

twodft commented 1 month ago

Oh I see. Others have reported this sort of thing, but there's really nothing we can do about that, the comment is definitely needed. Can you tell astro-compress to allow certain comments to remain in?

There should be some workarounds, Astro-compress use html-minifier-terser to minify HTML, so ignoreCustomComments should works;

Also, based on my test, wrap Server island components with a parents elements can also avoid this issue.

Adding a notice for this in the doc will be helpful.

matthewp commented 1 month ago

Wrapping with a parent element would break CSS expectations. Many frameworks use comments as markers, so minfiers that do this are going to break many tools. I'll contact astro-compress to see if they will change their defaults.

matthewp commented 1 month ago

Going to close as I don't think there's anything we can reasonably do about this. Filed an issue with astro-compress: https://github.com/advanced-astro/astro-compress/issues/141

matthewp commented 1 month ago

There might be a solution: https://github.com/livewire/livewire/commit/b666304652006f3491987c92d53724bc14035d6a

Going to test this.

NikolaRHristov commented 1 month ago

Hi, @twodft, @florian-lefebvre, @matthewp. Original astro-compress dev here. This might fix it https://github.com/PlayForm/Compress/releases/tag/AstroCompress%2Fv2.3.2

We've added https://github.com/PlayForm/Compress/blob/2b9713fb8097bcddf384e3abba15d8fbccd4cdba/Source/Variable/HTML/html-minifier-terser.ts#L20-L21

Would like to know if there are any other Astro comments that you would like to keep.

NikolaRHristov commented 1 month ago

Also if possible, @twodft can you open an issue in https://github.com/PlayForm/Compress if this happens again?

matthewp commented 1 month ago

@NikolaRHristov Thanks but we've fixed this in #12090

withastro / astro