Skip to content

more effect ordering stuff #10958

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
Merged
merged 3 commits into from
Mar 27, 2024
Merged

more effect ordering stuff #10958

merged 3 commits into from
Mar 27, 2024

Conversation

Rich-Harris
Copy link
Member

some suggested changes to #10949. WIP. two categories of failures:

  • infinite effect loops
  • onDestroy appears to happen in backwards order now? see ondestroy-deep

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 27, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 9737e02

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rich-Harris
Copy link
Member Author

Honestly I'm a little surprised at the current expected ondestroy-deep output. It matches Svelte 4, but it seems weird that outer onDestroy callbacks (and onMount callback return functions) would run before inner ones. You could definitely make the case that the behaviour in this PR makes more sense.

@Rich-Harris Rich-Harris marked this pull request as ready for review March 27, 2024 20:35
@Rich-Harris
Copy link
Member Author

not finished, but merging so we can continue on a single branch

@Rich-Harris Rich-Harris merged commit 1579278 into better-effect-scheduling Mar 27, 2024
@Rich-Harris Rich-Harris deleted the better-effect-scheduling-2 branch March 27, 2024 20:36
Rich-Harris added a commit that referenced this pull request Mar 28, 2024
@trueadm @Rich-Harris
fix: improve effect sequencing and execution order (#10949)
d50b766 
* WIP

* WIP

* address bad merge conflict

* add test

* fix issues

* remove debugger

* increase count

* increase count

* something different

* change

* change

* try it

* better comment

* remove deadcode

* move to continue

* fix tests

* add optimization

* unksip test

* Update packages/svelte/src/internal/client/dom/elements/bindings/this.js

Co-authored-by: Rich Harris <[email protected]>

* Update packages/svelte/src/internal/client/dom/elements/bindings/this.js

Co-authored-by: Rich Harris <[email protected]>

* Update packages/svelte/src/internal/client/dom/elements/bindings/this.js

Co-authored-by: Rich Harris <[email protected]>

* remove import

* add changeset

* tweaks

* code golf

* remove pre effects

* more effect ordering stuff (#10958)

* WIP

* i guess this change makes sense?

* simplify

* delete unused code

* delete pre_effect

* note to self

* tidy up

* typos

* style tweaks

* style tweaks

* improve reactive statement handling

* no return needed

* let prettier put everything on a single line

* style tweaks

* var

* failing test

* fix test

* fix ordering

* simplify

* ondestroy

* working

* note breaking change

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Rich-Harris