Skip to content

feat: allow dom elements as svelte:element this attribute #15477

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

Closed
wants to merge 3 commits into from

Conversation

paoloricciuti
Copy link
Member

@paoloricciuti paoloricciuti commented Mar 9, 2025

Closes #15475

I think overall is a reasonable request, and it could be useful to allow for even more integration with the JS ecosystem. I can't really think of a downside except that maybe could be a bit weird if you have children AND pass an html element with childrens.

It still needs

  • docs
  • related pr in language-tools to allow for the new type (i think)

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

Copy link

changeset-bot bot commented Mar 9, 2025

🦋 Changeset detected

Latest commit: 7903a35

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

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

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

@paoloricciuti
Copy link
Member Author

paoloricciuti commented Mar 9, 2025

Some thoughts by @dominikg

potential conflicts if both svelte:element and dom thing have children. or what happens if dom thing is attached already, yeet it there?

evil: what happens if you pass a bind:this obtained ref to svelte:element ?

less technical but this changes the definition of svelte element from "creates element" to "creates or attaches element" is that too ambiguous and a new name like svelte:attach could help avoid that?

And my answer

As I've said in the pr (I think) it currently acts as mount in that if it has already some children it appends after them.

It's more interesting the other cases: technically if it's from a different part of the dom It would move here, and then append to it which is an interesting but probably too confusing behavior. The same is true for something obtained through bind:this (a more interesting question is what if you pass the same element to this and bind:this).

The name is an interesting question too although the above possible source of confusion make me question the feature all together

We could however add runtime validation to only accept nodes that are not already connected.

@lassebomh
Copy link

We could however add runtime validation to only accept nodes that are not already connected.

I think thats a very good idea, and if you really wanted to "move" something, you could pull it out of the DOM before targeting it.

a new name like svelte:attach could help avoid that?

Could also be named after mount's target option, like <svelte:target this={...} /> . For me i think either way is fine, since this isn't really a in-your-face feature, but still very useful for integrating non svelte packages into Svelte.

@paoloricciuti
Copy link
Member Author

I've added the runtime validation...still a bit unsure about the feature however. But it could help integrating libraries with svelte which is something I like.

@adiguba
Copy link
Contributor

adiguba commented Mar 9, 2025

Hi,

I also think it would be better to have a distinct name : <svelte:element> is compatible with SSR, but here we are 100% on client side.

This would allow us to have something like this, which would be completely ignored during SSR (apart from any hydration marker)

<svelte:attach this={createElement()} />

And it could event be better if he could accept promises, since some libraries need to be dynamically loaded only on client side to avoid errors on server :

async function createElement() {
    const lib = await import("...);
    return lib.createElement();
}

@paoloricciuti
Copy link
Member Author

I also think it would be better to have a distinct name : <svelte:element> is compatible with SSR, but here we are 100% on client side.

Technically you could do something like

<svelte:element this={browser ? div : "div"} />

To have it SSR...dunno if there's a use case for that tho.

I'm not super worried about the promise thing because you will likely load that in an effect/on mount and assign to a variable and this would be the first case where we await something for the user I think

@theetrain
Copy link
Contributor

theetrain commented Mar 9, 2025

Looking at the original problem:

<script>  
  let { ...rest } = $props();
  
  const view = new EditorView({
    doc: value,
  });
  
  let wrapper;
  
  onMount(() => {
    wrapper.appendChild(view.dom)
  })
</script>

<!--
  How can we reactively apply `...rest` attributes
  to the `view.dom` parent and not this wrapper?
-->
<div class="wrapper" bind:this={wrapper} {...rest}></div>

This is solvable in userland by using an $effect.pre() to set props onto CodeMirror's EditorView parent as attributes:

$effect.pre(() => {
  if (editorAttributes === undefined || editorAttributes === {}) return
		
  const attributes = Object.entries(editorAttributes)
		
  if (attributes.length === 0) return
    Object.entries(editorAttributes).forEach(([key, val]) => {
      view.dom.setAttribute(key, val)
    })
})

Demo: https://svelte.dev/playground/236b091b6e0647bbab1ae9404526d145?version=5.22.6

I am also concerned about allowing <svelte:element> to be used for client-only rendering; I expect elements to be server-rendered. On the topic of providing some sort of container for inserting elements via JS, maybe <svelte:fragment> can be given this new job, but I'm not sure.

I prefer using .appendChild() as close to vanilla as possible that way it operates the same with and without Svelte. An element needs to be on the page as an insertion point anyway, even if it's document.body, such as in the CodeMirror Basic Editor example.

@lassebomh
Copy link

To be frank, no, this doesn't solve the issue. What the proposal suggests is currently impossible:

There is no [functional] way of placing elements created via document.createElement into the root of a component or use Svelte features/syntax on them.

Your solution suffers from the same problem that prompted me to make proposal: You're forced to have a wrapper element, and potentially sync props, which has its own set of problems. From the issue:

I have plenty of potential workarounds. So far, none of them address the problem in a way thats nice, hence the feature proposal.

So far we've mostly agreed that this feature could be pretty useful under the right circumstances and I'd be happy to expand on workarounds in the issue, but I'd keep the topic of this PR to issues with the proposal itself.

I get your concerns about svelte:element, but it's not a fragment, it's a literal element, and should support adding onclick, style and everything else. If we can't use svelte:element, then I'd prefer svelte:dom, svelte:target or svelte:attach (although could be confused with #15000), but maybe it makes sense to do the bike shedding after its decided if this feature should make it in, one way or another.

@7nik
Copy link
Contributor

7nik commented Mar 10, 2025

<svelte:element bind:this={node} class="foo" style="color:red"/>

will override classes and styling set by the library, won't it?

Having a node controlled by both Svelte and 3rd-party library at the same time doesn't sound good. This is why libraries accept a parent + customizations params - it draws a clear border between the framework's and the library's controlled nodes.

@paoloricciuti
Copy link
Member Author

<svelte:element bind:this={node} class="foo" style="color:red"/>

will override classes and styling set by the library, won't it?

Having a node controlled by both Svelte and 3rd-party library at the same time doesn't sound good. This is why libraries accept a parent + customizations params - it draws a clear border between the framework's and the library's controlled nodes.

Mh yeah this is another drawback...I'm less and less inclined to consider this feature request...it has some niceties but it also has a bunch of quirks which would be difficult to document.

@adiguba
Copy link
Contributor

adiguba commented Mar 10, 2025

Having a node controlled by both Svelte and 3rd-party library at the same time doesn't sound good.

I don't think it's a problem.
Both cases exist depending on how the library is designed...

Of course, if we use an external library, it should be used according to how it was designed.
But just because it's wrong with some library doesn't mean it's wrong with all of them.

After all, this :

<svelte:element bind:this={node} class="foo" style="color:red"/>

is nothing but an equivalent to this code :

node.className = "foo";
node.style.cssText = "color: red";

So, if you have a node, you can already do that...

@lassebomh
Copy link

I don't think it's a problem.
Both cases exist depending on how the library is designed...

I agree. Without trying to strawman the argument, it's kinda similar to disallowing <div bind:this={...}> in case someone tried to pass it to something that modified it. We're attaching an element that we know is from outside the walled garden, and I'd wager that regular Svelte users won't know this feature exists unless they need it, in which case they're most likely aware of the side effects.

If CodeMirror did in fact expose a host option and I used that instead, I'd have to be aware of the exact same quirks, except the component would be much less readable (because view could only be created after onMount). It happens that they don't, and Svelte has no way of working with an HTML element at the component root, so we're forced into a worse situation. All that said, I fully understand being weary about it. Without the isConnected constraint or the nieche-ness of the use case, I'd be much more inclined to agree.

@Rich-Harris
Copy link
Member

I really don't think we should add this. Not only because of the SSR issue and the ambiguity over child content, but because the motivating use case is for a single element to be controlled by two separate systems, which is a recipe for unnecessary confusion.

To me this seems like a perfect use case for attachments (or, in today's world, actions). It means you need a wrapper element but that's a good thing, because responsibilities are clearly delineated.

@paoloricciuti
Copy link
Member Author

I really don't think we should add this. Not only because of the SSR issue and the ambiguity over child content, but because the motivating use case is for a single element to be controlled by two separate systems, which is a recipe for unnecessary confusion.

I'm much more concerned about SSR and ambiguity...the reason today you need a wrapper element is because of code mirror not because of svelte: if code mirror accepted a host instead of a parent actions/attachments would suffer the same "problem"

@Rich-Harris
Copy link
Member

And I would bet good money that that's why CodeMirror doesn't accept a host element — because it's a bad idea! Marijn would spend half his time responding to bugs caused by declarative frameworks nuking stuff CodeMirror needs to control. Adding an API specifically to work around that healthy separation will only cause problems.

In my view any of the three issues (SSR, child content, split control) would be a good reason not to add this API.

@paoloricciuti
Copy link
Member Author

In my view any of the three issues (SSR, child content, split control) would be a good reason not to add this API.

Agree on this...I was surprised by the flexibility it allowed initially but this feels niche and risky enough so I'm in favor of closing it

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.

Allow HTML elements in <svelte:element this={...} />
6 participants