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

Support v-slot #1383

Merged
merged 12 commits into from
Jan 8, 2020
Merged

Support v-slot #1383

merged 12 commits into from
Jan 8, 2020

Conversation

lmiller1990
Copy link
Member

@lmiller1990 lmiller1990 commented Dec 22, 2019

Working on #1261

  • support the new v-slot syntax!
  • need to bump to Vue v2.6
  • likely a component combining the new and old scoped syntax will fail; we need to cover that case, it should be pretty easy, though.
  • will need to rebase and fix my commits, the commit convention is wrong
  • not sure if this is the best place to do this logic; likely we should have a simple util function to handle all the null checks.
  • added two tests; one with SFC, one without. Likely we can nuke one... just wanted to be thorough.

@lmiller1990 lmiller1990 changed the title Wip: Improve slots Wip: Support v-slot Dec 22, 2019
@JessicaSachs
Copy link
Collaborator

On the surface, this looks good to me. Like you said, we probably want to refactor it into a function to hide the nested checks... but aside from that 👍

I'm curious if $parent will always be present -- can root elements have scopedSlots? I'm particularly thinking of components constructed with createElement.

@lmiller1990
Copy link
Member Author

@JessicaSachs will need to investigate more to answer you questions, but got around to doing a refactor.

My commit messages aren't styled property, will use the squash feature on Github to have a single nice commit message when we merge this. Might even be fine to merge as is and come back to address - thoughts? Maybe we don't cover every edge case, but should at least cover some.

@lmiller1990 lmiller1990 changed the title Wip: Support v-slot Support v-slot Jan 3, 2020
@lmiller1990
Copy link
Member Author

@JessicaSachs I tried a component like this:

<template>
  <slot name="blah" />
</template>

To see what would happen to parent, since parent should be undefined. Turns out you cannot do this, Vue will throw an error:

(Emitted value instead of an instance of Error)
    Error compiling template:

    <slot name="newSyntax" />

    - Cannot use <slot> as component root element because it may contain multiple nodes.

I added a check for good measure, just in case.

@JessicaSachs
Copy link
Collaborator

@lmiller1990 yeah you can’t make slots the root element when you use template functions, but you can with createElement and a render function.

@lmiller1990
Copy link
Member Author

Hm, maybe I am missing something - does it make sense for the root element (eg App.vue most of the time) to have a <slot /> in the first place? That would imply you are passing something from above the root to be rendered in the slot.

I couldn't figure out how to write a render function with slots - if you can write a basic example I would be happy to add a test and see what happens.

Thanks for the thorough review... slots feel like the most edge-casey of anything in VTU. We definitely need to get this right.

Copy link
Collaborator

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. This looks good.

I spent a few hours trying to break this using render functions on root vue components and other use cases and couldn't get it to break. I do a bunch of edgecase-y things in my prod app and this seems to handle all of it.

@afontcu
Copy link
Member

afontcu commented Jan 8, 2020

Great job mate! 🙌

@lmiller1990
Copy link
Member Author

Going to add one more test around v-slot then merge this up, should be done by the end of the week

@JessicaSachs JessicaSachs merged commit e4d8c2c into dev Jan 8, 2020
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.

4 participants