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

fix: correctly set custom element props #14508

Merged
merged 4 commits into from
Dec 10, 2024
Merged

fix: correctly set custom element props #14508

merged 4 commits into from
Dec 10, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Dec 2, 2024

fixes #14391

In #13337 the "when to set as a property" logic for custom elements was adjusted. A bug was introduced during this, and it consists of several parts, of which the latter I'm not sure what's the best solution, hence opening this to discuss.

The problem is that during set_custom_element_data, get_setters is the only check done to differentiate between setting the value as a prop (has a setter) or as an attribute (doesn't have a setter).

The solution is to take into account whether or not the custom element is already registered, and defer getting (and caching) its setters until then. Instead, fall back to a "an object is always set as a prop" heuristic.

Old description

Part 1 of the bug is that this only checks the setters of the prototype, but you can also add setters to the instance type. Furthermore, custom elements may not have instantiated yet when we come across them to set the property, which means the setters may not have been added yet. That's why we can cache the setters only once element.isConnected is true.
This part is fixed in this PR.

But part 2 of the bug is not fixed through that yet: What if you set an object to the custom element while it is not instantiated yet / there is no such setter yet? Then the fix will be useless. Example:

<script lang="ts">
	import { onMount } from 'svelte';

	class CustomElement extends HTMLElement {
		constructor() {
			super();
			this.attachShadow({ mode: 'open' });
		}

		connectedCallback() {
			console.log('b#m');
			Object.defineProperty(this, 'image', {
				set: (value) => {
					this.shadowRoot.innerHTML = typeof value + '|' + JSON.stringify(value);
				}
			});
		}
	}

	onMount(async () => {
		customElements.define('wc-image', CustomElement);
	});

	let image = $state({ foo: 'bar' });
</script>

<wc-image {image}></wc-image>

image is set too soon and therefore set as an attribute. Even if we had handling inside the web component to pick up a prop from before the element was upgraded it would fail.

I therefore think we need to revisit the logic change from #13337 and either for custom elements set this as a property if it's not a string even without a setter, or go back to what it was before completely.

Thoughts?

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Sorry, something went wrong.

WIP
Copy link

changeset-bot bot commented Dec 2, 2024

🦋 Changeset detected

Latest commit: eabcb3a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Playground

pnpm add https://pkg.pr.new/svelte@14508

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14508-svelte.vercel.app/

this is an automated message

@dummdidumm dummdidumm marked this pull request as ready for review December 5, 2024 14:34
@OvidijusParsiunas
Copy link

Hi @dummdidumm and other Svelte maintainers.

Thankyou for raising this PR!

I own two web component repos (deep-chat/active-table) and consumers of both have raised issues of not being able to pass values as properties when using the new Svelte version 5. One of the web components is using a custom implementation of HTMLElement and the other is using Lit, but the problem appears to be there for both.

Concurring to your description, the setters of my libraries are set on instance types, hence Svelte 5 does not pass values as properties and attempts to pass them via attributes which simply ends up passing a string containing [object Object].

This PR should hopefully take care of the problem and we are very much looking forward to its release!

Thankyou for your hard work!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@dummdidumm dummdidumm merged commit a2539cf into main Dec 10, 2024
11 checks passed
@dummdidumm dummdidumm deleted the ce-props-fix branch December 10, 2024 21:56
@github-actions github-actions bot mentioned this pull request Dec 10, 2024
@nevil2
Copy link

nevil2 commented Dec 12, 2024

Thank you very much. My TopoEditor web component is working again.

@OvidijusParsiunas
Copy link

OvidijusParsiunas commented Dec 12, 2024

Likewise! All is good again! 🎉 🎉 🎉 🎉

@ceifa
Copy link
Contributor

ceifa commented Jan 23, 2025

I think this PR has a regression where the initial state of bindable props doesn't work when passed to a custom element inside a browser extension context.

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

Successfully merging this pull request may close these issues.

Svelte v.5 passes a file as a string (rather than as a file) into a web component prop
6 participants