-
Notifications
You must be signed in to change notification settings - Fork 439
fix: mismatched scoped / light slots / data #5131
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,22 +25,33 @@ const bConditionalSlot = esTemplateWithYield` | |||||||
| if (isLightDom) { | ||||||||
| const isScopedSlot = ${/* isScopedSlot */ is.literal}; | ||||||||
| const isSlotted = ${/* isSlotted */ is.literal}; | ||||||||
| const slotName = ${/* slotName */ is.expression}; | ||||||||
| const lightGenerators = lightSlottedContent?.[slotName ?? ""]; | ||||||||
| const scopedGenerators = scopedSlottedContent?.[slotName ?? ""]; | ||||||||
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); | ||||||||
| const generators = isScopedSlot ? scopedGenerators : lightGenerators; | ||||||||
|
|
||||||||
| // start bookend HTML comment for light DOM slot vfragment | ||||||||
| if (!isSlotted) { | ||||||||
| yield '<!---->'; | ||||||||
|
|
||||||||
| // scoped slot factory has its own vfragment hence its own bookend | ||||||||
| if (isScopedSlot) { | ||||||||
| // If there is slot data, scoped slot factory has its own vfragment hence its own bookend | ||||||||
| if (isScopedSlot && generators) { | ||||||||
| yield '<!---->'; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| const generators = lightSlottedContent?.[${/* slotName */ is.expression} ?? ""]; | ||||||||
| if (generators) { | ||||||||
| for (const generator of generators) { | ||||||||
| yield* generator(contextfulParent, ${/* scoped slot data */ isNullableOf(is.expression)}); | ||||||||
| for (let i = 0; i < generators.length; i++) { | ||||||||
| yield* generators[i](contextfulParent, ${/* scoped slot data */ isNullableOf(is.expression)}); | ||||||||
| // Bookends after all but last scoped slot data | ||||||||
| if (isScopedSlot && i < generators.length - 1) { | ||||||||
| yield '<!---->'; | ||||||||
| yield '<!---->'; | ||||||||
|
Comment on lines
+49
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really bizarre behavior! I wonder what explains it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait I think I see – it's because of the double bookend? So the double bookend logic for the start/end is handled elsewhere? It might be good to add a comment explaining this!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep exactly. Added a note to explain that! |
||||||||
| } | ||||||||
| } else { | ||||||||
| // If there were mismatched slots, do not fallback to the default | ||||||||
| } else if (!mismatchedSlots) { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd love to understand why mismatched slots have any impact here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment here to explain: This is required for parity with engine-core which resets children to an empty array when there are children (mismatched or not). Because the child nodes are reset, the default slotted content is not rendered in the mismatched slot case. See here. I'm guessing this is a sort-of-bug but fixing it would mean a v1/client behavior change and I don't think it's worth it.... |
||||||||
| // If we're in this else block, then the generator _must_ have yielded | ||||||||
| // something. It's impossible for a slottedContent["foo"] to exist | ||||||||
| // without the generator yielding at least a text node / element. | ||||||||
|
|
@@ -53,8 +64,8 @@ const bConditionalSlot = esTemplateWithYield` | |||||||
| if (!isSlotted) { | ||||||||
| yield '<!---->'; | ||||||||
|
|
||||||||
| // scoped slot factory has its own vfragment hence its own bookend | ||||||||
| if (isScopedSlot) { | ||||||||
| // If there is slot data, scoped slot factory has its own vfragment hence its own bookend | ||||||||
| if (isScopedSlot && generators) { | ||||||||
| yield '<!---->'; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
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.
For mismatched slot case an error is logged in v1 / client (we don't throw). Error is logged non-production only. There was no precedence in v2 for this so do we need to do anything? Throwing would be too much, it needs to recover.
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.
Logged errors are essentially meaningless, so I don't think we need to do anything here. It would be nice to have error logging somewhere in v2, but also not a big deal, since presumably folks can see the same logged error in their browser DevTools.
(As an aside, it should probably be a
console.warntoo! #3692)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.
Thanks, that is more elegant