Skip to content

fix: properly clean up component lifecycle order issue #16684

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: properly clean up component lifecycle order issue #16684

wants to merge 1 commit into from
+13 −5

Conversation

fabhari
Copy link
Contributor

@fabhari fabhari commented Aug 28, 2025 •
edited
Loading

  • move effect cleanup to happen before new creation in key function
  • ensure proper lifecycle order. destroy old component -> create new component
  • so now the order is script -> mount -> abort -> destroy -> script -> mount

fixes #16563

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@fabhari
fix: properly clean up component lifecycle order issue
0081950 
- move effect cleanup to happen before new creation in key function
- ensure proper lifecycle order. destroy old component -> create new component
- so now the order is script -> mount -> abort -> destroy -> script -> mount
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 28, 2025

🦋 Changeset detected

Latest commit: 0081950

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@16684

@fabhari
Copy link
Contributor Author

fabhari commented Aug 28, 2025

playground
Without Fix : REPL

With PR Fix : REPL

dummdidumm
dummdidumm requested changes Aug 28, 2025
View reviewed changes
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you, but we can't do this unfortunately. This is basically the same as #16624 (which was for dynamic components), so it suffers the same problem when in async mode. So we have to find a different solution, not exactly sure how that looks like yet

@fabhari
Copy link
Contributor Author

fabhari commented Aug 29, 2025 •
edited
Loading

hi @dummdidumm I have written async test code but don't see any ordering issues. Could you provide a concrete playground example of the specific problem this change prevents?
REPL

@fabhari fabhari requested a review from dummdidumm August 29, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dummdidumm dummdidumm Awaiting requested review from dummdidumm

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Order of execution between script and teardown
2 participants
@fabhari @dummdidumm