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

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

Open
PatrickJS opened this issue May 8, 2022 · 10 comments
Open

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

PatrickJS opened this issue May 8, 2022 · 10 comments
Labels
core Mitosis Core enhancement New feature or request help wanted Extra attention is needed

Comments

@PatrickJS
Copy link
Contributor

PatrickJS commented May 8, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

sahilmob commented May 9, 2022

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

@PatrickJS
Copy link
Contributor Author

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
Copy link
Contributor Author

PatrickJS commented May 9, 2022

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
Copy link
Contributor

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

@PatrickJS
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@PatrickJS PatrickJS changed the title add new fields to MitosisComponent type: slots core: add new fields to MitosisComponent type: slots May 19, 2022
@PatrickJS PatrickJS added enhancement New feature or request help wanted Extra attention is needed core Mitosis Core labels May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Mitosis Core enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants