fix: mismatched scoped / light slots / data#5131
Conversation
| const slotName = ${/* slotName */ is.expression}; | ||
| const lightGenerators = lightSlottedContent?.[slotName ?? ""]; | ||
| const scopedGenerators = scopedSlottedContent?.[slotName ?? ""]; | ||
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); |
There was a problem hiding this comment.
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.
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.warn too! #3692)
There was a problem hiding this comment.
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); | |
| const mismatchedSlots = isScopedSlot ? lightGenerators : scopedGenerators; |
There was a problem hiding this comment.
Thanks, that is more elegant
jhefferman-sfdc
changed the title
nolanlawson
left a comment
nolanlawson
left a comment
There was a problem hiding this comment.
This looks reasonable to me, and it's definitely a huge improvement. (4 tests fixed!) I'm a little concerned though that I don't understand why these fixes worked. Figuring out exactly why engine-dom/engine-server behave the way they do and then adding thorough comments would be super helpful.
| const slotName = ${/* slotName */ is.expression}; | ||
| const lightGenerators = lightSlottedContent?.[slotName ?? ""]; | ||
| const scopedGenerators = scopedSlottedContent?.[slotName ?? ""]; | ||
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); |
There was a problem hiding this comment.
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.warn too! #3692)
| if (isScopedSlot && i < generators.length - 1) { | ||
| yield '<!---->'; | ||
| yield '<!---->'; | ||
| } |
There was a problem hiding this comment.
This is really bizarre behavior! I wonder what explains it.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Yep exactly. Added a note to explain that!
| const slotName = ${/* slotName */ is.expression}; | ||
| const lightGenerators = lightSlottedContent?.[slotName ?? ""]; | ||
| const scopedGenerators = scopedSlottedContent?.[slotName ?? ""]; | ||
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); |
There was a problem hiding this comment.
| const mismatchedSlots = (isScopedSlot && lightGenerators) || (!isScopedSlot && scopedGenerators); | |
| const mismatchedSlots = isScopedSlot ? lightGenerators : scopedGenerators; |
| } | ||
| } else { | ||
| // If there were mismatched slots, do not fallback to the default | ||
| } else if (!mismatchedSlots) { |
There was a problem hiding this comment.
I'd love to understand why mismatched slots have any impact here.
There was a problem hiding this comment.
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....
Thank you for reviewing! I added some additional clarification comments and made the ternary simplification for the mismatchedSlot condition. |
| yield '<!---->'; | ||
| yield '<!---->'; |
There was a problem hiding this comment.
| yield '<!---->'; | |
| yield '<!---->'; | |
| yield '<!----><!---->'; |
3 participants
Details
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-17555947