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 5 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.
I would rephrase this sentence. Shadow DOM is not an enforcement, it is just the default.
This proposal is not a toggle, because it is not a 1-1 mapping between shadow DOM and light DOM. It will certainly require developers to rewrite partially their component. I see this proposal as a brand new type of components (with new requirements and restrictions).
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.
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.
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 would remove this section, it doesn't bring much to this proposal.
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.
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.
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 here gives examples of how other frameworks integrate with Shadow DOM. I don't think they related to this proposal.
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.
I'm a bit concerned about exposing light DOM via
MacroElement
. I can break my concerns into two groups: 1) deviation from other WC frameworks, and 2) potential class proliferation.Deviation from other WC frameworks
Here is how other frameworks handle light DOM:
Stencil
Fast
Lit
So we have 2 frameworks opting for decorators, and another opting for an overridable method. LWC would be the only WC library using separate classes (unless I missed one).
Potential class proliferation
In the future,
we may want to support closed shadow roots(edit: my mistake, didn't realize we already did 🤦 ). Would that be another class?There is also a proposal for "open stylable roots" (WICG/webcomponents#909). Would this be another one?
In that proposal, there is even some talk of
attachShadow()
eventually looking like this:If the options bag for
attachShadow
continues to grow like this, then we could end up with a combinatorial explosion of classes to handle every possible case.My personal preference would be for decorators or mixins, but mostly I'm just concerned about choosing classes here.
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 think the original reason we chose a different class was to emphasize that these are different kinds of components because the programming model is different. I'm not sure if we still feel that is the case.
I'm curious how the createElement API will change which currently takes the mode.
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.
@tedconn Ah my mistake, I didn't even know we supported closed shadow roots. 🙂 It seems that
createElement
would be the right place to put the options bag (if it becomes a big bag!).So it looks like my concerns about class proliferation might not apply, since we only plan on having
MacroElement
for this one case (light DOM). But then why not have light DOM as an option increateElement
?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 we need an API on the component, so component authors can toggle, not on the engine. Something like this would be nicer though, right?
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 discussed in the past 2 options, using a static field or using a brand new base class. Until the decorator proposal goes back to stage 3, I don't think it is safe to introduce a brand new decorator.
The main advantage of introducing a brand new class over static fields is around component inheritance. You can shadow (by inadvertence or intentionally) a base class static field. It means that you can turn a Shadow DOM component into a Light DOM one (and vice-versa) by extending from it. Such override can't be done when inheriting from a different base class.
In the following example, let say that we introduce a new
shadowDOM
static boolean to indicate whether the component rendered in the Light DOM or in the Shadow DOM. It is something we should discourage.Now that you speak about class proliferation, I am more favorable to adding a signal via a static field.
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 discussion with the team, we landed on the static field rather than
MacroElement
, correct? AFAICR, we concluded:Is this right? @pmdartus @abdulsattar
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.
You are right.
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.
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.
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.
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.
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.
:global
is not a standard CSS selector. I don't think it worth referencing this here.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.
How do I reference the root element in a light dom 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.
With
LightningElement
, all the properties and methods on thethis
value interacts with the host element (this.getAttribute
,this.querySelector
,this.classList
). Andthis.template
allows to interact with the shadow root. However, LWC never exposes the host element itself to the component instance. You can get access to it, viathis.template.host
but it is more a hack than anything else.The same model is preserved with
MacroElement
. It is possible to interact with the host element using methods and properties on thethis
value. But there is currently no workaround (likethis.template.host
) to access the host element from aMacroElement
component instance.If accessing the host element is necessary, I would vote for making it available to both
LitghtningElement
andMacroElement
in a similar fashion. For example via ahost
getter.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.
way more detail necessary here. Does this mean that a component was authored in the LightDom but app requested it in the shadow dom for ex?
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 think many people are asking "if everything is light DOM then what purpose does Synthetic Shadow DOM serve? The way I tried to answer that question today is "there will always be components somehow in the shadow DOM so we will always need an emulation." but maybe I am wrong?
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 are trying to get out of this model, where some components will always need emulation.
Synthetic shadow purpose was created to emulate shadow DOM on browsers that don't support shadow DOM natively. As we are sunsetting IE11 support, we shouldn't need synthetic shadow anymore.
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 this section belongs to the proposal.
It overlaps more with @ravijayaramappa's work on Shadow DOM mixed mode.
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 would extract this section out of the RFC and move this content to a spike document.
The internal implementation might change over time, while the RFC should remain accurate on the long run.
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.