-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add option to make usage of shadow dom configurable #2516
Conversation
src/compile/render-dom/index.ts
Outdated
|
||
@init(this, { target: this.shadowRoot }, ${definition}, create_fragment, ${not_equal}, ${prop_names}); | ||
@init(this, { target: this${options.shadowDom ? '.shadowRoot' : ''} }, ${definition}, create_fragment, ${not_equal}, ${prop_names}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't guess I'm 100% confident about this. It makes the target itself... I assume it's okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. Yes this should work. I just had to modify the javascript generated by Svelte: set "this"as the target in the init() method of the component, and remove "this.attachShadow({ mode: 'open' });" from SvelteElement constructor() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldumaine, If the shadowDom option is disabled then the target will be this
and the constructor of SvelteElement
should get passed { use_shadow_dom: false }
which will make this.attachShadow({ mode: 'open' })
not be called.
src/compile/index.ts
Outdated
@@ -74,7 +79,7 @@ function get_name(filename) { | |||
} | |||
|
|||
export default function compile(source: string, options: CompileOptions = {}) { | |||
options = assign({ generate: 'dom', dev: false }, options); | |||
options = assign({ generate: 'dom', dev: false, shadowDom: options.customElement }, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defaults shadowDom
to whatever customElement
may be set to. So if it's true
, shadowDom
will be true
by default (which is the behavior we want for backwards compatibility).
src/compile/index.ts
Outdated
@@ -53,6 +54,10 @@ function validate_options(options: CompileOptions, warnings: Warning[]) { | |||
toString: () => message, | |||
}); | |||
} | |||
|
|||
if (!customElement && shadowDom) { | |||
throw new Error(`options.shadowDom cannot be true if options.customElement is false`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a little sanity check to save us from an invalid state.
src/parse/state/tag.ts
Outdated
@@ -132,7 +132,7 @@ export default function tag(parser: Parser) { | |||
? meta_tags.get(name) | |||
: (/[A-Z]/.test(name[0]) || name === 'svelte:self' || name === 'svelte:component') ? 'InlineComponent' | |||
: name === 'title' && parent_is_head(parser.stack) ? 'Title' | |||
: name === 'slot' && !parser.customElement ? 'Slot' : 'Element'; | |||
: name === 'slot' && !parser.shadowDom ? 'Slot' : 'Element'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another area I wasn't 100% sure on. From my reading slot
seems to be directly related to the shadow DOM. I assume that with a custom element we'd just want the slot
to be a regular element..
Update on this: Everything seems to be working except for slots. I haven't been able to figure it out yet, but slots don't seem to be rendering how I expect. Continuing to dig into it though. |
Okay @Rich-Harris, I'm officially stuck. It looks like this absolutely is an error and I just can't figure out what's causing the slots not to generate correctly. The failing does a pretty good job of highlighting the issue. It uses three files: Given these test filesmain.svelte <svelte:options tag="custom-element"/>
<script>
import './AsElement.svelte';
import ImportedElement from './AsImported.svelte';
</script>
<as-element>Hello</as-element>
<ImportedElement>world</ImportedElement> AsElement.svelte <svelte:options tag="as-element"/>
<h1><slot/></h1> AsImported.svelte <svelte:options tag="imported-element"/>
<h2><slot/></h2> I expect this output <custom-element>
<as-element><h1>Hello</h1></as-element>
<as-imported><h2>world</h2></as-imported>
</custom-element> Instead I'm getting <custom-element>
<as-element>Hello</as-element>
<h2></h2>
</custom-element> So the custom element Any advice would be much appreciated 🙏 |
@zephraph I've been exploring your changes as I'd also like to separate However, the fact that customElements cannot be used as Svelte components properly (noted in other issues particularly around the CSS) can be partially solved by changing this line svelte/src/compile/render-dom/index.ts Line 441 in 394a166
options.props to init in addition to the target .
|
@colinbate Svelte already has a notion of non-shadowDOM slots. I've got logic to switch between the two implementations here Thanks for the props tip though.. I didn't think about that. I'll play with it. 👍 |
It does have that notion, and it should be possible to get it to work, however I think it will take more than just swapping the parsing for I love working with native Svelte, but custom elements still feels like it is beta. |
As an update to anyone watching, I'm pretty stuck with this PR and @Rich-Harris is focused on Sapper at the moment. I'd be happy to pair with someone to try to push through. Just let me know! |
Argh. Every time I study web components in any detail, I'm freshly dismayed. I think @colinbate is right that this is a lot more involved than just changing the behaviour of <as-element>Hello</as-element> ...with shadow DOM and without shadow DOM is huge. In the first case, Since a lot of people have independently concluded that shadow DOM makes styling harder, rather than easier, some WC frameworks offer an option to disable it. I looked at Stencil to understand how they handle content distribution with <!-- this works as expected, if <as-element> doesn't use shadow DOM... -->
<as-element>Hello</as-element>
<script>
// ...but this nukes the <h1> inside the <as-element>
document.querySelector('as-element').innerHTML = `Hello`;
</script> In my opinion this makes a mockery of the notion that the distributed components are framework-agnostic — they're broken unless consumed within a Stencil app. I'm not convinced it's worth adding the same feature to Svelte. Unfortunately, if you want to be free of shadow DOM, you also lose As an aside, there's an important way in which Svelte components handle <some-component>
<p>this is created before some-component even instantiates</p>
</some-component> Svelte 2 worked that way, and it was a major hassle. In v3 we switched to Vue-like lazy rendering of content — in other words, the component controls it, not the consumer of the component. This also means that the content can be stamped out multiple times with different data, e.g. for virtual list components and the like. Perhaps we could have required component authors to wrap lazy content in The upshot of which is that we have something that superficially resembles the shadow DOM So I'm not sure what to do about this. I don't see a really good way forward. We could throw an error if Thoughts? |
Sounds pretty fair and in par with the specs |
Well, given this insight, I don't know that it's worth pursuing further at this point. I'll close and see if there's a way I can approach my original problem (share global styles) in a different way. Thanks for the insight @Rich-Harris. |
Patrick Nelson, on his npm package svelte-retag says that Could it be that things have changed since 2019 and the requested feature is now available along with I have no experience with svelte-retag, because I am working with svelte-tag, from which svelte-retag is forked and which now also works with svelte-5. On that way i can opt-out shadow-dom. I am happy with that since years. So, I have found my way, but I really hope that svelte, with the |
Now, i did some test, and found that
In both cases I was able to apply global styles to the components as expected without shadow dom, and found that slots worked well.
Details on github-issue P.S.: |
Fixes #1748.
This is a WIP PR. It's not done and I'm not entirely certain I'm on the right path, but it's a start.
Custom elements have more general support than does the shadow dom. While using shadow dom may be a good default, it's not always desirable.
This is a big change and it definitely could benefit from more tests. SSR, slots, config variations are all areas that I can think of off the top of my head.