Skip to content
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

Show compiler warning/error for slot attribute not at the top level #4703

Closed
Tracked by #2964
nolanlawson opened this issue Oct 25, 2024 · 4 comments · Fixed by #4711
Closed
Tracked by #2964

Show compiler warning/error for slot attribute not at the top level #4703

nolanlawson opened this issue Oct 25, 2024 · 4 comments · Fixed by #4711
Labels
enhancement Up for grabs Issues that are relatively small, self-contained, and ready for implementation

Comments

@nolanlawson
Copy link
Collaborator

nolanlawson commented Oct 25, 2024

    <x-inner>
        <div>
            <span slot="foo">I am the foo slot</span>
        </div>
    </x-inner>

In the case above, the developer is probably confused because they seem to be trying to slot something into the foo slot of <x-inner>, but since it's inside a <div>, the whole<div> is actually treated as the default slotted content, and the slot attribute is effectively ignored. This works in all three of native shadow, synthetic shadow, and light DOM slots.

There are valid cases where it's not at the top level, e.g.:

    <x-inner>
        <template lwc:if={isTrue}>
            <span slot="foo">I am the foo slot</span>
        </template>
    </x-inner>

In this case, the <span> would truly go into the foo slot since the <template lwc:if> is not a "real" element.

We should enumerate all the cases where the slot attribute is valid or invalid when not at the top level and warn for it. Potentially in the future we could even throw an error to be more explicit.

@nolanlawson nolanlawson changed the title Show compiler warning for slot attribute not at the top level Show compiler warning/error for slot attribute not at the top level Oct 25, 2024
@nolanlawson nolanlawson added the Up for grabs Issues that are relatively small, self-contained, and ready for implementation label Oct 25, 2024
@nolanlawson
Copy link
Collaborator Author

Doing a warning here is up for grabs, but doing an error is trickier so would probably have to be done with API versioning.

@nolanlawson
Copy link
Collaborator Author

The full list of Node types is here:

export type Node =
| Root
| ForBlock
| If
| IfBlock
| ElseifBlock
| ElseBlock
| BaseElement
| Comment
| Text
| ScopedSlotFragment;

Off the top of my head, the ones that are "valid" to have between the component (<x-inner> above) and the element with the slot attribute are If, IfBlock, ElseifBlock, ElseBlock. For ForBlock and ScopedSlotFragment I'm not sure.

@cardoso
Copy link
Contributor

cardoso commented Oct 25, 2024

@nolanlawson this warning would need to be ignored here due to fixtures added in #4689 right?

// TODO [#3331]: The existing lwc:dynamic fixture test will generate warnings that can be safely suppressed.
const shouldIgnoreWarning =
message.includes('LWC1187') ||
// TODO [#4497]: stop warning on duplicate slots or disallow them entirely (LWC1137 is duplicate slots)
message.includes('LWC1137') ||
message.includes('-h-t-m-l') ||
code === 'CIRCULAR_DEPENDENCY';
if (!shouldIgnoreWarning) {
throw new Error(message);
}

Example:

<div>
<div slot="foo">
I am the foo slot
</div>
</div>

@nolanlawson
Copy link
Collaborator Author

Yes, true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Up for grabs Issues that are relatively small, self-contained, and ready for implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants