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

Fix: 2 islands on 1 page are revived #197

Merged
merged 12 commits into from
Jun 7, 2022

Conversation

sylc
Copy link
Contributor

@sylc sylc commented May 18, 2022

Fix: #196

The island end comment node was removed before the call to node.nextSibling, stopping the walk. I have re-ordered the deletion and added a test.

tests/islands_test.ts Outdated Show resolved Hide resolved
tests/islands_test.ts Outdated Show resolved Hide resolved
@lucacasonato
Copy link
Member

Thanks for the fix. I think this should generally work, except in the case of nested islands. Nested islands will not work like you are expecting. In the test with <Inner> and <Outer>, the inner component is never directly hydrated. Instead it is hydrated as part of <Outer>. In this case this results in the correct behavior, but it may not always, as the props that are serialized into the HTML will not be used to revive the <Inner> island.

@lucacasonato
Copy link
Member

I guess this does fix nested islands like I described in #168, so we can probably land this. The only concern is that there is a little bit of inefficiency because we are shipping the props for the <Inner> component, even though we are not hydrating it.

@sylc
Copy link
Contributor Author

sylc commented May 21, 2022

I guess this does fix nested islands like I described in #168, so we can probably land this. The only concern is that there is a little bit of inefficiency because we are shipping the props for the <Inner> component, even though we are not hydrating it.

This intend of this PR is not really to fix #168 but to fix #196 (the reviving issue when 2 island are in one page). I added to test for the nested island because i though they would be needed in the future anyway and to make sure there was no regression.

nested island still don't work for example with a thin outer as per reported in #178

if you think, that the nested island test should be added in a PR with a more holistic way around the nested island issue, then they can be removed.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM.

I have removed the nested islands test for now.

Thanks for working on this.

@lucacasonato lucacasonato merged commit afafb7c into denoland:main Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Hydration fails on > 1 Island
2 participants