-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: smaller destructor chunk #8763
Conversation
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.
Also think that this makes sense, you shouldn't be relying on the order of operations here, and I don't see how you can since this isn't a component or action, just a simple html element which has no side effect of unmounting.
Still tagging @Conduitry to be extra sure
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.
After digging through the compiler, I was going to suggest the same change.
However, this is definitely a breaking change as DOM MutationObservers will now possibly fire in different orders.
If we want to make this non-breaking - how hard is it to preserve the order of removals and just batch those where elements are removed consecutively? Example: if (detaching) {
remove(f1);
remove(f2);
}
someOtherDestroyOperation();
if (detaching) {
remove(f3);
} (assuming we care about the order of operations, doubt it really matters for these) |
Shouldn't be that difficult, not sure we'd need to/if it's worth it. We can cut a release with it today and let people test it out? |
those things are probably nothing that someone finds in a next release. I'd say let's keep it as is, put it into the 4.0 release and if someone opens a bug about it then we can adjust it |
Some modest savings here and there, I'm trusting the runtime tests that the order of those statements won't matter.
HEADS UP: BIG RESTRUCTURING UNDERWAY
The Svelte repo is currently in the process of heavy restructuring for Svelte 4. After that, work on Svelte 5 will likely change a lot on the compiler aswell. For that reason, please don't open PRs that are large in scope, touch more than a couple of files etc. In other words, bug fixes are fine, but feature PRs will likely not be merged.
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests
npm test
and lint the project withnpm run lint