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: prevent island from re-rendering when using transition:persist #11915

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

azhirov
Copy link
Contributor

@azhirov azhirov commented Sep 3, 2024

This PR addresses the issue described in #11854.

Issue: In the swapBodyElement function, the props attribute of the custom element is being updated even when the new and old values are identical. This unnecessary update triggers rehydration, which can lead to unexpected behavior during transitions. swap-functions.ts#L82

When navigating to a different page containing the same component (island), the following method is triggered on the custom element astro-island:

attributeChangedCallback() {
    this.hydrate();
}

(Source: astro-island.ts#L204)

Changes

I introduced a check to compare the props attribute values between el and newEl. The update now only occurs if the values differ.

However, since these strings represent serialized objects, there is a potential concern about whether a simple string comparison is sufficient, considering that the property order within the serialized objects might vary.

Testing

I tested the changes by navigating between pages that contain the same component (island) to ensure that the props attribute is not unnecessarily updated, preventing redundant rehydration. The component state now persists correctly across transitions without triggering unnecessary rehydration.

The testing was conducted using the code available at https://stackblitz.com/edit/github-jhuzfh-jxxerz. This environment allowed me to reproduce the issue and verify the effectiveness of the fix.

No new tests were added since this change addresses a specific issue with existing behavior, and the existing test suite sufficiently covers the transition and hydration logic. However, I verified that all existing tests pass, ensuring that there are no regressions caused by this fix.

Docs

This change does not affect user-facing behavior or API, so no updates to the documentation are needed.

Copy link

changeset-bot bot commented Sep 3, 2024

🦋 Changeset detected

Latest commit: 42cd338

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 3, 2024
Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

Hi @azhirov 👋🏼, thank you very much for providing a fix for #11854!

That looks good to me: If I understand your solution correctly, we don't update the props if they haven't changed.

From the original issue:

The usecase where a component state is persisted and still react to props change is broken for vue, svelte and solid

Could you please add an e2e test using a Vue component (similar to the existing island-two.astro/island-one.astro pair) that shows that updating props now works without errors for persisted components when the props do change?

@johannesspohr
Copy link
Contributor

I opened a PR (#11946) to fix the Vue issue, that should also work in case of changed props.

@matthewp
Copy link
Contributor

matthewp commented Sep 9, 2024

lgtm, aside from needing an E2E like @martrapp suggested.

@martrapp
Copy link
Member

Hi @azhirov and @johannesspohr: How about merging your two PRs? #11915 looks as if it would work with other frameworks beyond Vue, and #11946 does have the e2e test!

@johannesspohr
Copy link
Contributor

Hi @azhirov and @johannesspohr: How about merging your two PRs? #11915 looks as if it would work with other frameworks beyond Vue, and #11946 does have the e2e test!

If I'm not mistaken, this PR only handles situations where the props are not changed, but if they change, it's still broken (at least I couldn't find any code to update the props of islands other than React)
I implemented said update logic for Vue in my PR to the best of my knowledge, but it still needs approval. The other integrations (Svelte, Solid, Alpine) would also require a similar implementation. If it is working for all FE frameworks, the change from this PR would probably not be needed anymore.

@martrapp
Copy link
Member

@johannesspohr added test for Svelte, Vue, and Solid through his framework specific solutions. Thank you very much for this!

Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

As @johannesspohr pointed out, this PR will not address the root cause of re-hydration. But it might reduce the number of cases where people would be forced to manually add transition:persist-props.

@martrapp martrapp merged commit 0b59fe7 into withastro:main Sep 17, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 17, 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.

4 participants