Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Light DOM Support #44
RFC: Light DOM Support #44
Changes from 21 commits
cb55a94
72fe402
a306839
9bf37b0
bad70a8
6b6572e
f791aec
990bc6a
dea55cf
0a87dea
a9b1c55
ea27928
3a62c00
fbcab6b
b2df06f
36a77b8
8313490
340f4b9
a00f719
653b680
846f6b9
74f6da6
458cc0f
619721a
798268f
a3f6d71
c8a76f0
e735d3e
3460e3a
baa79bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
As Dean said in DRB today, UIP has the same (near-term future) requirements around theming and branding for LEX and LWR apps as Communities.
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 believe the LEX requirements are the same as what Communities is requesting because the number of actors in the system is very different. Let's discuss offline.
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.
@hsterlingsfdc Could you make sure to bring the LWC team into the loop when this will be discussed/designed? I would like to better understand the LEX and LWR requirements in this regard.
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.
Image content: Remove "Today" and "Where we want to be" label. The RFC should not present points of view but rather preset facts.
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.
The list of invariants and constraints is missing. From the live discussion on this RFC today it became obvious that there are many you're working with.
There are many others that need to get captured here. This'll bring visibility to the rationale driving this design.
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'm still fuzzy about:
what I see in the PR is that the content of the slot is flatten, so the slot element itself never gets rendered.
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 remind me of a presentation of a good friend (https://www.youtube.com/watch?v=HYl7ReNB5TA), basically, this API is wrong from the design point of view, but it is also wrong when you look at it with the backward compatibility lenses (which I understand is very edge case, but still problematic).
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 think it's too late to change this, the API is not considered stable today. We debated between @nolanlawson and @abdulsattar this couple of weeks ago. I purposefully made the API name generic because I had in mind to reuse the same
shadow
static field in the future to developers configure the shadow root:@caridy I know that you tinkered with something similar in the past. Where did your previous research lead you?
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 argument in favor of
shadow = false
overlight = true
: web developers tend to think of "shadow DOM" as a fancy new thing, and "light DOM" as the default. In fact, "light DOM" was just "the DOM" for decades.So developers are more likely to immediately understand
shadow = false
compared tolight = true
, since they understandshadow
as this weird new bleeding-edge-standards thing that they need to have a separate mental model for, as opposed tolight
, which is just how the web worked for decades.One counter-argument to that, though, is that developers working with LWC have probably had to acquaint themselves with
shadow
already, as it's the default in LWC. So I see good arguments both ways.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.
When dealing with boolean values, as a best practice, the default should be
false
. It looks like this was maybe modeled after Stencil's API which disables shadow DOM by default, whereas we enable it by default. I would prefer to go with something likestatic shadowDisabled = true
orstatic disableShadow = true
.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.
Interesting. The idea was to make the
shadow
field accept more complex objects in the future. For example, we would also use theshadow
field to override the shadow DOM mode:FYI @caridy as you were working on something similar to configure LWC components.
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.
Agree that defaulting to
true
is weird, but to me thefalse
versions feel weirder: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.
Agreed with @nolanlawson on the point above. If we were to change the flag name and use a different mechanism to configure the shadow root, I would vote for @caridy's
static renderToLightDOM: true
.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 really only care that the default is
false
, but I'll just point out that the double-negative usually isn't a concern because you don't need to set the property to get the default behavior.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.
After thinking about this some more, I'm ambivalent about
renderToLightDOM
. Although the light DOM doesn't exist in synthetic shadow, both light DOM and shadow DOM do exist in native shadow. We want to indicate that we're going to take what would normally have been rendered in the light DOM and slotted into the shadow DOM, and do some magic without attaching a shadow root.renderToLightDOM
doesn't seem to capture that, but I don't have any other suggestions at the moment.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 is a good point.
shadow = false
implies "no shadow at all", which is true. Whereaslight = false
implies "no light at all," which is not true because it's actually a mix of light and shadow (due to slots).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.
Does it influence components created after the
shadow
property's value is changed?Should this value be made non-writable at runtime?
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.
Because of how we're handling Babel loose mode, I'm not sure it's possible to enforce this. I could be wrong though.
As-is though, if
shadow
is redefined after theconstructor()
is called, then the change would have no effect.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.
Hm, so it looks like it's possible, but we have two options:
shadow
a second time.FooElement
runs itsconstructor()
, disallow settingshadow
again.The downside of the first is that users may legitimately want to set
shadow
more than once (complex app logic) before calling the constructor. The downside of the second is that it's a bit odd than a single instance of a class would have an impact on the entire class.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.
In the current implementation updating the
shadow
attribute after the component is created has no effect. The attribute is read during the construction phase and then cached in theVM
for the lifetime of the component.Making the static field non-writable doesn't provide much guaranty if the field is defined as a getter:
We can for example cache the value
shadow
value after theFooElement
instantiation and reuse it for all the futureFooElement
instantiations. I am not personally in favor of making the first component instantiation special.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 affects builders if it is not a real slot. This may mean every single builder (LAB is only one of the many builders Salesforce has) has to add special code to process components dragged into another component's light dom slot.
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.
Unfortunately it would be impossible to use real slots for light DOM, because slots as defined by the web platform are a part of shadow DOM.
Whatever we decide on for "light DOM slots," it will not be the same as shadow DOM slots.
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.
Thanks. Follow-up question: Today, we have internal LWC container components like tabset and accordion that are exposed in LAB (via an aura wrapper). ISV LWC container components (which are just components that contain slots that the builder understands) are on the roadmap. It sounds like the recommendation is for ISVs to build these with Light DOM, since they are general purpose and just meant to house other content.
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.
It's really up to each use case. I think we would need to draft a longer document about the strengths and weaknesses of light DOM versus shadow DOM.
For instance, even a tabset component may want to protect its shadow content from being unintentionally styled by the consumer, so shadow DOM may be an appropriate choice there. Also a tabset will inevitably use slots, meaning that there could be performance or timing differences between shadow and light DOM. (E.g. eager versus lazy – a tabset containing 10 slots that only renders 1 might still pay the cost of rendering the other 9, if slots are eager, as they are in native shadow DOM.)
But yes, in general, I think your intuition is correct. Tabsets and accordions don't themselves contain sensitive content, so they could be good candidates for light DOM.
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 believe the issue is broader than builders: any system that generates LWC components needs to be updated if the LWC syntax differs in how slotted content is provided to a Light DOM-enabled component.
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.
Other thoughts:
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.
Side note: any system that generates LWC components will need to be updated to respect the
no-shadow
directive anyway.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.
Side note:
On the consumer side in the
app.html
, the example below is using the standardslot
attribute. From the compiler standpoint, it is impossible to know if the<x-slottable>
uses the light DOM or the shadow DOM slotting algorithm.I was under the original impression that we could also ask developers to use a custom directive to indicate to the template compiler to use the light DOM slotting algorithm to avoid this branching at runtime. For example using
slot:no-shadow
instead ofslot
. That said I came to the conclusion that it is not possible.While this custom directive works well with named slots I don't see how this would work with the default slot. In the example below I don't see how it is possible to indicate that the text content should go to the light DOM instead of the shadow DOM.
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 is a good point. I don't know how to resolve it, so perhaps slots should be moved to a separate RFC entirely.
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.
After discussing this in a meeting, it seems the only way to resolve the above example is to move the logic to the runtime. I.e. we have to figure out at runtime rather than compile time whether a slot is shadow or light.
The only workaround I can think of would be to make a hard requirement on light DOM slots that they have some top-level element that we can mark:
This isn't great for developer ergonomics, but it would allow us to figure this out at compile time.
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.
We decided that whether slots are light or shadow will be determined at runtime.
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.
Now that I think about this, I don't think the
slot
attribute should be rendered at runtime in the light DOM.We decided not to render the
<slot>
element at runtime to prevent potential issues where the light DOM<slot>
connects to the parent shadow root. If we were to keep theslot
attribute at runtime for the light DOM we could also run into unexpected slotting behaviors at runtime when mixing shadow DOM slot and light DOM slots.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.
Does this run into the same problem where the consumer component needs to know whether
<x-slottable>
is a light DOM element or not? If so, it would maybe argue forno-shadow:slot="foo"
orlwc:slot="foo"
or something like that.Then again as you said above, that does not solve default slots. Unless we force something like
lwc:slot="default"
.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.
Please define container.
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.
We need to think more about the low-code use cases and the admin persona.
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.