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

Custom elements are not "cleaned up" when the consuming application removes an element. #8191

Closed
Pixelatedza opened this issue Jan 12, 2023 · 4 comments · Fixed by #8457
Closed

Comments

@Pixelatedza
Copy link

Pixelatedza commented Jan 12, 2023

Describe the bug

Hey guys, it appears that custom-elements don't get cleaned up fully when a parent application removes them from the DOM. The issue croped up because of a crash in a child component, specifically a modal that is animated in.

At first I thought it was just the subscriptions to the stores that weren't being removed appropriately. However, in a more complex example, I have nested components with conditional rendering based on the store value, and this logic still gets executed. At least, I assume so, since the components crashing would not be rendered if said logic wasn't applied.

  • Site
    • custom-element
      • nested child with logic (subscribes to store)
        • conditionally rendered child (crashes because of missing stylesheet reference).

I assume this is because the components or their subscriptions were not fully cleaned up when the parent removed them from the DOM.

In my project I can work-around the error, for now, by removing the animation. Changing
<div class={`modal ${size}`} transition:fly={{ y: 1000, duration: 500 }}>
to
<div class={`modal ${size}`}>

The error: Uncaught (in promise) TypeError: Cannot read properties of null (reading 'insertRule'). I'm guessing this is because the stylesheet has been removed/cleaned up by either svelte or the browser, but the component still attempts to do it's thing (animate in), because the store value has changed.

Attached a video showing what I'm seeing for what it's worth. I'm running the minaml repo I've linked below.

svelte-bug.mov

Reproduction

Here is a minimal reproduction. You can run svelte as usual, yarn dev. It's in the /vite-project directory. Then open the index.html in the root of the repo in a browser.

If you toggle the content on and click on "change store value", the value of the store will be printed as many times as the content was shown.

https://github.com/Pixelatedza/min-svelte-wc-bug-demo

Logs

No response

System Info

System:
  OS: macOS 11.6.7
  CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  Memory: 915.37 MB / 16.00 GB
  Shell: 5.8 - /bin/zsh
Binaries:
  Node: 16.15.1 - ~/.nvm/versions/node/v16.15.1/bin/node
  Yarn: 1.22.19 - ~/.nvm/versions/node/v16.15.1/bin/yarn
  npm: 8.11.0 - ~/.nvm/versions/node/v16.15.1/bin/npm
Browsers:
  Chrome: 109.0.5414.87
  Firefox: 108.0.2
  Safari: 15.6
npmPackages:
  svelte: ^3.54.0 => 3.55.1

Severity

blocking all usage of svelte

@jrmoynihan
Copy link

jrmoynihan commented Jan 15, 2023

I've been running into the same error:
Uncaught (in promise) TypeError: Cannot read properties of null (reading 'insertRule')

The error is in the /internal module, inside the create_rule function:

// svelte/internal/index.js

function create_rule(node, a, b, duration, delay, ease, fn, uid = 0) {
    const step = 16.666 / duration;
    let keyframes = '{\n';
    for (let p = 0; p <= 1; p += step) {
        const t = a + (b - a) * ease(p);
        keyframes += p * 100 + `%{${fn(t, 1 - t)}}\n`;
    }
    const rule = keyframes + `100% {${fn(b, 1 - b)}}\n}`;
    const name = `__svelte_${hash(rule)}_${uid}`;
    const doc = get_root_for_style(node);
    const { stylesheet, rules } = managed_styles.get(doc) || create_style_information(doc, node);
    if (!rules[name]) {
        rules[name] = true;
        stylesheet.insertRule(`@keyframes ${name} ${rule}`, stylesheet.cssRules.length);
    }
    const animation = node.style.animation || '';
    node.style.animation = `${animation ? `${animation}, ` : ''}${name} ${duration}ms linear ${delay}ms 1 both`;
    active += 1;
    return name;
}

In my case, I have a client-side component (a tooltip) being created through an action on a node that is transitioning in, and rendered inside an {#await} block.

@Pixelatedza
Copy link
Author

I did some digging today. I must apologise, it was only after I dug deeper and understood the cause of this, that I realised that there are a few issues that are related to this.

In short, the $destroy is not called when custom elements are removed from the DOM "disconnected", this is intentional. The callback passed to unMount will still be called, that's what the PR above added.

If you need a quick work around for your project, you can use the below code to explicitly call $destroy when the elements disconnect. Just make sure it is run before any elements are defined.

import { SvelteElement } from "svelte/internal";
const _disconnectedCallback = SvelteElement.prototype.disconnectedCallback;
SvelteElement.prototype.disconnectedCallback = function () {
  _disconnectedCallback.call(this);
  this.$destroy();
};

Can we consider adding a compiler option that will add $destroy to the disconnect callback? This leaves it up to the programmer to decide if it's relevant to his project. Potentially also adding it to svelte:options as well.

Something I'm still struggling to figure out is what are the ramifications of doing so? The HTML spec and others have mentioned it doesn't mean the same thing to custom elements, but as far as I can tell, it means no longer in the same DOM. In sveltes case, since the components are isolated by nature, does it matter if it gets destroyed/recreated everytime?

@baseballyama baseballyama added this to the 4.x milestone Feb 26, 2023
@chronicIntrovert
Copy link

I am also running into this issue regardless of custom-element. In my code I have a div that's conditionally rendered by the presence of a $modal store not being null. This div also has transition:fly property and errors on trying to close by setting the store back to null.

dummdidumm added a commit that referenced this issue May 2, 2023
This is an overhaul of custom elements in Svelte. Instead of compiling to a custom element class, the Svelte component class is mostly preserved as-is. Instead a wrapper is introduced which wraps a Svelte component constructor and returns a HTML element constructor. This has a couple of advantages:

- component can be used both as a custom element as well as a regular component. This allows creating one wrapper custom element and using regular Svelte components inside. Fixes #3594, fixes #3128, fixes #4274, fixes #5486, fixes #3422, fixes #2969, helps with sveltejs/kit#4502
- all components are compiled with injected styles (inlined through Javascript), fixes #4274
- the wrapper instantiates the component in `connectedCallback` and disconnects it in `disconnectedCallback` (but only after one tick, because this could be a element move). Mount/destroy works as expected inside, fixes #5989, fixes #8191
- the wrapper forwards `addEventListener` calls to `component.$on`, which allows to listen to custom events, fixes #3119, closes #4142 
- some things are hard to auto-configure, like attribute hyphen preferences or whether or not setting a property should reflect back to the attribute. This is why `<svelte:options customElement={..}>` can also take an object to modify such aspects. This option allows to specify whether setting a prop should be reflected back to the attribute (default `false`), what to use when converting the property to the attribute value and vice versa (through `type`, default `String`, or when `export let prop = false` then `Boolean`), and what the corresponding attribute for the property is (`attribute`, default lowercased prop name). These options are heavily inspired by lit: https://lit.dev/docs/components/properties. Closes #7638, fixes #5705
- adds a `shadowdom` option to control whether or not encapsulate the custom element. Closes #4330, closes #1748 

Breaking changes:
- Wrapped Svelte component now stays as a regular Svelte component (invokeing it like before with `new Component({ target: ..})` won't create a custom element). Its custom element constructor is now a static property named `element` on the class (`Component.element`) and should be regularly invoked through setting it in the html.
- The timing of mount/destroy/update is different. Mount/destroy/updating a prop all happen after a tick, so `shadowRoot.innerHTML` won't immediately reflect the change (Lit does this too). If you rely on it, you need to await a promise
@dummdidumm
Copy link
Member

Closed via #8457, to be released in Svelte 4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants