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

Implement <slot> #787

Merged
merged 11 commits into from
Aug 28, 2017
Merged

Implement <slot> #787

merged 11 commits into from
Aug 28, 2017

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Aug 26, 2017

This addresses #763. {{yield}} is turned into <slot> at the parser level, which means we can remove yield-related stuff from the codebase altogether. The only difference anyone will notice is that component contents are now wrapped in <slot>...</slot>

It's already proving to be a more efficient solution — we no longer need to create a new block for component contents, and can instead simply append elements which is cheaper.

TODO:

  • Named slots
  • component.slots
  • Fallback content
  • Validation (no slots or slot attributes named default, no dynamic slot attributes on elements, no attributes other than name on <slot> elements, no duplicate <slot> elements within components, no nested <slot> elements, no <slot> elements in each blocks)
  • Deprecate {{yield}}
  • Update docs and create examples for the REPL
  • Update the tests using {{yield}}

@Rich-Harris Rich-Harris changed the title Implement <slot> [WIP] Implement <slot> Aug 26, 2017
@Rich-Harris Rich-Harris changed the title [WIP] Implement <slot> Implement <slot> Aug 27, 2017
@Rich-Harris
Copy link
Member Author

Alright, this is ready now if anyone wants to take a look. The three remaining TODOs are post-release tasks.

@m59peacemaker
Copy link
Contributor

m59peacemaker commented Aug 27, 2017

I cloned the repo and did npm run build, then installed the directory into a project. svelte-plugin-rollup is using that svelte and I can see slot related things in my bundle, but the slots aren't getting anything put into them. For the most basic usage, I should be able to just trade out {{ yield }} for <slot></slot>, right?

@Rich-Harris
Copy link
Member Author

'It works for me' is the most unhelpful reply, but, uh, it works for me:

screen shot 2017-08-27 at 12 27 43 pm

This is the code for that: https://gist.github.com/Rich-Harris/cbded6e9a638f0a76b8c95577c79a660

I should be able to just trade out {{ yield }} for <slot></slot>, right?

You shouldn't even need to do that — it'll replace {{yield}} with <slot/> for you.

@m59peacemaker
Copy link
Contributor

Alas, I was looking in all the wrong places! I forgot I had begun considering ./docs a separate project with its own build and dependencies, but it is also using svelte. My package was being built with slots, but the demo that uses it wasn't.

I think there is a bug, though. The following works:

<!-- excerpt from Modal.html -->
<slot name="scrim">
  <div>just some text</div>
</slot>

<!-- SomewhereElse.html -->
<Modal>
  <div slot="scrim"><p>Whatever html you want</p></div>
</Modal>

But if I add attributes to the element in the fallback content, I get this error at runtime.

<slot name="scrim">
  <div class="foo">just some text</div> <!-- will break due to attribute on element -->
</slot>
docs.js:401 Uncaught TypeError: Cannot set property 'className' of undefined
    at Object.hydrate (docs.js:401)
    at Object.create (docs.js:389)
    at Object.create (docs.js:881)
    at Object.update (docs.js:807)
    at Demo._set (docs.js:243)
    at Demo.set (docs.js:220)
    at HTMLButtonElement.click_handler (

@m59peacemaker
Copy link
Contributor

Also note that if there is fallback content with attributes and the fallback content is used (nothing put into the slot), things are fine. Therefore, I suppose that when something is put into the slot, the element in the fallback content is appropriately undefined, but there is undesired code running to apply attributes to the unneeded and non-existent element.

@Rich-Harris
Copy link
Member Author

Yeah, it just needs to wrap the hydration code in a conditional. I basically have it working locally, just need to tweak some stuff. Good catch, thanks

@Rich-Harris
Copy link
Member Author

Should be fixed now.

@Rich-Harris Rich-Harris changed the title Implement <slot> [WIP] Implement <slot> Aug 28, 2017
@Rich-Harris
Copy link
Member Author

Eek. Putting the WIP tag back on. Turns out that <slot> elements should not actually be rendered, but replaced with either the corresponding 'light DOM' or their fallback content.

A corollary: we can't have the component.slots API. Slotted content must be declared at instantiation time like so:

new Thing({
  target: document.body,
  slots: {
    default: someNode,
    foo: someOtherNode
  }
});

Discovered this because I'm working on an app that has some flexbox styles, and the presence of the <slot> (between the display: flex element and its descendants) was borking things up.

@m59peacemaker
Copy link
Contributor

I was doing this to get around a circular dependency

const thing = new Thing({ target })
this.slots.default = makeSlotStuff(thing)

The same can still be done with one extra step

const slotElement = document.createElement('div')
const thing = new Thing({ target, slots: { default: slotElement })
slotElement.appendChild(makeSlotStuff(thing))

I bet this will change and/or be fixed by your current work, but in case it matters, the latter example currently breaks on destroy due to these lines in unmount

Failed to execute 'appendChild' on 'Node': The new child element contains the parent.

if (slot_content_default) {
    while (slot_default.firstChild) appendNode(slot_default.firstChild, slot_content_default);
}

@Rich-Harris
Copy link
Member Author

Fixed — <slot> is no longer rendered.

One consequence — the slotted content needs anchors, so that we can unmount it. In that way it's similar to if/each blocks, except that we need an anchor before and after, and similar to raw tags except that we can use comments instead of <noscript> elements. When we get round to addressing #637, we should consider this at the same time, since the comments aren't always necessary (we can use neighbouring nodes).

@m59peacemaker that should fix the problem you identified! 🤞

Another thing that occurs to me: fallback content is another excellent reason to work on #749. If we can analyse the entire app, we can statically determine whether or not a component needs fallback capability for each slot.

@Rich-Harris Rich-Harris changed the title [WIP] Implement <slot> Implement <slot> Aug 28, 2017
@Rich-Harris Rich-Harris merged commit 6fad3cb into master Aug 28, 2017
@Rich-Harris Rich-Harris deleted the gh-763 branch August 28, 2017 23:29
@m59peacemaker
Copy link
Contributor

That JS API destroy() bug is still occurring. https://svelte.technology/repl?version=1.34.0&gist=f6e34033c880e29ffa1bfbbcf6e5d5b7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants