-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: more event handling tweaks #12383
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
- ensure we only have a single document listener per event+runtime - add `<svelte:body>` listeners to `before_init` similar to the document/window elements - move some code into `events.js` where it belongs
🦋 Changeset detectedLatest commit: bf45ba2 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 |
|
I'm unsure about this. Ideally we'd increment a counter for each root, and remove the document listeners when the count goes to zero – otherwise we're retaining a document event listener for no reason. |
|
There's no way that we can intercept unmounting, because it's not required to unmount if everything that has references to the instance is garbage collected.so the trade-off is lingering document listeners vs duplicate document listeners if you mount more than once. |
|
@dummdidumm What do you mean by intercepting unmounting? If someone doesn't unmount their root, it will cause a memory leak. Tested it just now with this case locally. |
|
Yeah, people need to unmount their components properly, you can't rely on GC. Though whether or not they bother to do that, given I added the counter, and if people do unmount their components listeners are once again removed, but if they don't then there's now a limit to the damage they can do, so this seems to be the best of both worlds |
<svelte:body>listeners tobefore_initsimilar to the document/window elementsevents.jswhere it belongsfollow-up to #12376 which isn't released yet, therefore no changeset
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint