-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix: derived reactivity and perf regressions #17362
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
Conversation
🦋 Changeset detectedLatest commit: 5ef1d01 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
2e7ecea to
8ff5006
Compare
8ff5006 to
3268f7d
Compare
6250ba5 to
3b7d3c7
Compare
3b7d3c7 to
f00cdee
Compare
|
@paoloricciuti would love if you could review/merge this one too :) As you can see, this regression is the source of many head aches! |
|
@hmnd yeah I already took a look...I think the fix makes sense but I'm not super familiar with the logic of the connected deriveds as I would like so I would not feel confident to review and merge this on my own. I've already shared it in the maintainers chat yesterday tho so hopefully we can get this merged soon. Thanks for your effort 🧡 |
086aa13 to
6ad1227
Compare
6ad1227 to
a21bb4c
Compare
…oying_effect handling
|
Made some progress — swapped out the unit test of the bug in #17352 for a component-based test, since these tend to be a little easier to reason about and debug in our sandbox. Would love to see if we can whittle it down further, but will come back to it later |
Rich-Harris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, think we're good to go now. thank you!
|
Thank you for your work refining this to a usable fix! Glad I could contribute something meaningful, and hope to be able to be of more help in the future. |
fixes sveltejs/kit#15059, fixes #17342, fixes #17111, fixes #17323, fixes #17352
#17105 resulted in a few regressions to how deriveds with no dependencies are treated. At worst, it broke reactivity for deriveds in branch effects, but I realized it also casued perf regressions like #17342. Both are now addressed, with tests to match.
To summarize, the issues I've fixed are:
effect_tracking()is false there)Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint