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

feat: smaller destructor chunk #8763

Merged
merged 5 commits into from
Jun 20, 2023
Merged

Conversation

gtm-nayan
Copy link
Contributor

@gtm-nayan gtm-nayan commented Jun 20, 2023

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

  • 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.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@gtm-nayan gtm-nayan marked this pull request as ready for review June 20, 2023 06:26
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.

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

@dummdidumm dummdidumm added this to the 4.x milestone Jun 20, 2023
Copy link
Contributor

@trueadm trueadm left a 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.

@dummdidumm
Copy link
Member

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)

@gtm-nayan
Copy link
Contributor Author

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?

@dummdidumm
Copy link
Member

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

@dummdidumm dummdidumm merged commit 914529f into version-4 Jun 20, 2023
7 checks passed
@dummdidumm dummdidumm deleted the smaller-destructor-chunk branch June 20, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants