BuilderIO / mitosis

Write components once, run everywhere. Compiles to React, Vue, Qwik, Solid, Angular, Svelte, and more.
https://mitosis.builder.io
MIT License
12.52k stars 555 forks source link

core: add new fields to MitosisComponent type: slots #325

Open PatrickJS opened 2 years ago

PatrickJS commented 2 years ago
export const createMitosisComponent = (
  options?: Partial<MitosisComponent>,
): MitosisComponent => ({
  '@type': '@builder.io/mitosis/component',
  imports: [],
  inputs: [],
  meta: {},
  state: {},
  children: [],
  hooks: {},
  context: { get: {}, set: {} },
  name: options?.name || 'MyComponent',
  subComponents: [],
  ...options,
});

a component needs to have all named slots available and I think it should be added here. and inside of children we need Slot nodes

'@type': '@builder.io/mitosis/component',
  slots: {
    mySelectorName: createMitosisComponent({})
  },
  children: [
    createMitosisNode({
      name: 'Slot',
      properties: {
        name: 'mySelectorName'
      } 
    });
  ],
steve8708 commented 2 years ago

great idea. only thing is this should probably be on @builder.io/mitosis/node right? aka when you have

<Layout slotFoo={<Bar />} />

it would become

    {
      "@type": "@builder.io/mitosis/node",
      "name": "Layout",
      "meta": {},
      "properties": {},
      "slots": {
        "foo": { "@type": "@builder.io/mitosis/node", name: "Bar" ... }
      },
      "children": []
    }

this would be a great eslint rule to eventually add too (cc @sahilmob), essentially that

<div foo={<Jsx />} />

is not allowed, you can only put jsx expressions as binding values if the binding name starts with slot

PatrickJS commented 2 years ago

yeah it depends on what version of slots we go with but the one with slotFoo makes more sense because it's less magic compared to

<Layout>
  <Slot foo={<Bar />} />
</Layout>
steve8708 commented 2 years ago

ah yes I see makes sense. I could see that working too, or also if we did like this we wouldn't need any JSX parser updates, would work OOTB

<Layout>
  <Slot name="foo">
     <Bar />
  </Slot>
</Layout>

that may be simplest in the short term as a result. then you can just get the slots with a helper, like

cost slots = getSlots(jsonTree);

children will already be parsed as expected. or I guess could also be something like

<Layout>
  <Bar slot="foo" />
</Layout>

and would similarly work ootb

I'm open minded here though, no strong opinion on my side, would be nice to do it as conventionally as possible (I forget how vue/svelte/etc handle slots lately)

steve8708 commented 2 years ago

we also have the ability to use $ attribute for special things too, since they are valid JSX but not valid HTML, like

<Bar $slot="foo" />

so there is no prop name collision, if we liked that direction

sahilmob commented 2 years ago

@steve8708 Sure thing. would love to add an eslint rule in the future 🚀 🚀 🚀

PatrickJS commented 2 years ago

yeah what I was trying to avoid was the slot/prop collision. So I'm not sure if we should make it look more like react or html

to make it look more like html

<Layout>
  <Slot name="foo">
     <Bar />
  </Slot>
</Layout>

to make it look more like react

<Layout slotBar={<Bar />} />

If we add $ to the mix it would allow us to have these directives

<Layout>
  <Bar $slot="bar"/>
</Layout>

function Layout(props) {
  return (
    <>
      <Slot $bar/>
    </>
  )
}

and react

<Layout
  $slotBar={<Bar/>}
>
</Layout>

function Layout(props) {
  return (
    <>
      {props.$slotBar}
    </>
  )
}

I was thinking of adding slotBar as a prop so it doesn't look like magic when you refer to it from props

function Layout(props) {
  return (
    <>
      <Slot name={props.slotBar}/>
    </>
  )
}

which would make more sense to react devs

PatrickJS commented 2 years ago

I think it makes more sense to leave $ for any API the dev wants in a plugin. If they create a plugin for tailwind it will allow any mitosis dev to know this is a macro created by their team vs Mitosis API

<Header $w24 $pt6 $bgRed={isFailed}>
steve8708 commented 2 years ago

ah yes collision avoidance makes sense, agreed with everything here 👍

PatrickJS commented 2 years ago

thinking about it more. the Mitosis way is to support both since we support both ways with <For> and .map()

so I'll add support for both

<Layout>
  <Slot name="foo">
     <Bar />
  </Slot>
</Layout>

function Layout(props) {
  return (
    <>
      <Slot name="foo" />
    </>
  )
}
function Layout(props) {
  return (
    <>
      <Slot name={props.slotFoo} />
    </>
  )
}

and

<Layout
  slotFoo={<Bar/>}
/>

function Layout(props) {
  return (
    <>
      {props.slotFoo}
    </>
  )
}

^ I'll add support for this react-way then support the html-way version after

PatrickJS commented 2 years ago

I think we need optional linters to enforce the react-way vs html-way. For example one codebase may want to enforce every loop uses <For> vs .map. We want the react-way to work but provide warnings for them to change it in a PR. So they may want to do the same for Slots. either to enforce react-way or html-way. This depends on the team if they understand react more or angular/vue more