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

Bug: slots with data #1333

Open
Marvin-Brouwer opened this issue Jan 19, 2024 · 21 comments
Open

Bug: slots with data #1333

Marvin-Brouwer opened this issue Jan 19, 2024 · 21 comments

Comments

@Marvin-Brouwer
Copy link

I need a slot in a list in which I can pass around the data of it's parent.
So for example, with a component like this:

import { Slot, Layout, For } from "@builder.io/mitosis";

export default function MyComponent(props) {
  const example = [ 

    "asdf",
    "dsfdsdf"
  ]

  return (
    <div>
      
      <For each={example} >
        {
          (item) => (
            <Slot name={props.slotPage} item={item}></Slot>
          )
        }
      </For>
    </div>
  );
}

I'd like this ouput in Vue

<template>
  <div>
    <template :key="index" v-for="(item, index) in example">
      <slot name="page" :item={item}></slot>
    </template>
  </div>
</template>

<script>
import { defineComponent } from "vue";

export default defineComponent({
  name: "my-component",
});
</script>

In vue you can do: <slot name="page" :item={item}></slot>, however I can't seem to figure out how to make mitosis do this.
The documentation doesn't state this as a gotcha, but it also doesn't state how to do this.

If I need to make a feature request for this, please just tell me and close the ticket.

Thanks,


https://mitosis.builder.io/?outputTab=G4VwpkA%3D&code=JYWwDg9gTgLgBAbzgZQDYRgGjgGQIYCeEArlnAGLRwC%2BcAZlBCHAEQACARscKgCYCmUAHTAIAehDAYEAM7AZLANwAoZfwAekWHAF08xVPDrEAdgGMYok3ACyBAMJNIJ%2FiZgAKMIzAyAlImU4ODMIExl4DTxwVH44AF44AG04VSCgljwZXjoWTEC0ll4ZOiLslnyAXVS4KH4YYihrd3yggB5eYAA3AD4WtL62yig4fjwzAAs4hEjo%2FlpetMWApaX3KX4QfzjuuGaVldawbta0DDgTKP4prwgfIRl0GAAFPABzObh1kCmv6mOxU4wf5HAZLXygoLUUGtMRDBZpGEdHr5XwqahAA%3D%3D%3D&inputTab=FYZwHkA%3D

@samijaber
Copy link
Contributor

This is a bug. Here we generate these slots:

if (slotName === 'default') {
return `<slot>${renderChildren()}</slot>`;
}
return `<slot name="${stripSlotPrefix(
slotName,
SLOT_PREFIX,
).toLowerCase()}">${renderChildren()}</slot>`;

We are missing the slots' properties and bindings, which are generated by this function:

const getBlockBindings = (node: MitosisNode) => {

I believe you should be able to reuse that function. If not, feel free to modify it as needed

@Marvin-Brouwer
Copy link
Author

I'm not sure how to "reuse that function", do you mean when creating a bugfix?

@samijaber
Copy link
Contributor

Yes, I am describing the process of contributing a fix to Mitosis itself 😄

@Marvin-Brouwer
Copy link
Author

Marvin-Brouwer commented Jan 19, 2024

AH, check.
I might take some time somewhere next week to do so.

I see react and svelte also aren't binding this, is this something you'd want?
And for example for react, would this mean it would output like <p>{slotExample(value)}<p/>
or would we maybe want to include a <Slot> component for react?
And do we know of more components that might need an update?

@Marvin-Brouwer Marvin-Brouwer changed the title Question: slots with data Bug: slots with data Jan 19, 2024
@Marvin-Brouwer
Copy link
Author

@samijaber I pulled down the repo and I immediately have failing tests:
image
Is there anything I need to configure?

@samijaber
Copy link
Contributor

There might be a problem with the configuration of the repo. Do the tests pass if you run yarn ci:build in the root?

@Marvin-Brouwer
Copy link
Author

Yes that seems to work out fine.
So running yarn test in the ~/packages/core folder doesn't work as expected.

Additionally, I now have 22 modified files after running yarn ci:build in the root.
Are you on unix or mac by any chance?
All of the changes appear to be whitespace.

@Marvin-Brouwer
Copy link
Author

I added some editorconfig to lock the newlines on unix line endings and now tests don't create new updates. So, I'm sure that was it.
I'll use yarn ci:build to validate my work for now and leave yarn test in the ~/packages/core folder out of scope for now.

I've done some digging and react seems to want you to use any kind of dynamic component like so:

<p>DynamicComponent({ prop: value })</p>

So I'd propose to change the slot implementation in react to something similar.

<ul>
  {items.map((item) => (
    <li>{ slotExample({ item }) }</li>
  ))}
</ul>

However, that would mean the regular slots will probably be better off looking like:

<p>{ slotRegular() }</p>

Are you okay with that?

I'll focus on vue first, then I'll look at svelte and then I'll tackle react.
I'll have a look at other formats after that, but I recon I'll need some time to get the vue fix in first.

Let me know if you have any questions, concerns or other remarks.

@Marvin-Brouwer
Copy link
Author

Marvin-Brouwer commented Jan 19, 2024

@samijaber I'm having a hard time understanding this part:
image

It looks like that in the case of a nameless slot, when there is anything in json.bindings the vue template mechanism should be used.
However, these don't seem to be specified in the snapshot tests.
How is this supposed to behave? And what about when someone would add a <Slot key="whatever" ... on their component? Should it also handle that key property?

I added some test scenarios to make it consistent.
With my current changes it looks like this:
image
This doesn't seem to behave consistent, and I'd like to fix that.
However, I'd like to know the intended behavior before I attempt to make it consistent.

@Marvin-Brouwer
Copy link
Author

Hi,

@samijaber I've figured some things out and made some progress but I'm kind of stuck right now.
When generating the snapshots somehow there's a type definition generated:
image

I can't find where this happens.
Could you please point me to the block of code that is responsible for generating these propType definitions?

Thanks

@samijaber
Copy link
Contributor

These definitions are stored in the types key, and the name of the type (Props in this case) is stored in propsTypesRef:

types?: string[];
propsTypeRef?: string;

which is then consumed by each generator, like here:

${json.types && options.typescript ? json.types.join('\n') : ''}

@samijaber
Copy link
Contributor

If you are concerned about the slot prefix added to slots in React, that is being fixed right now in #1334

@Marvin-Brouwer
Copy link
Author

No I'm not, I converted react slots from a field to function so data can passed just like in vue, svelte, etc.

However, the type still generates JSX.Element instead of () => JSX.Element or (slotProps) => JSX.Element
So, I'd like to take a stab at rectifying that.

@samijaber
Copy link
Contributor

I just looked at the snapshot you screenshotted:

import type { JSX } from '../../../../jsx-runtime';
type Props = {
[key: string]: string | JSX.Element;
slotTesting: JSX.Element;
};

It is actually an error to import the Mitosis JSX.Element here. For now, the best we can do is make this slotTesting: any, because Mitosis does not properly handle generating element types across all frameworks.

So it is safe to ignore this type or change it to any in the .raw.tsx file if you prefer.

@Marvin-Brouwer
Copy link
Author

Marvin-Brouwer commented Jan 24, 2024

Ah, check. I see now.
I was convinced these types were generated for React since it's the only framework that doesn't have some kind of slot mechanic.
But this makes sense to me.

I'll continue with solid then.

I may have some backwards compatibility questions when I'm done.
But I'll first see how far I can get this to work across all output types.

@samijaber
Copy link
Contributor

@Marvin-Brouwer I just re-read your implementation suggestion and realized it could cause breaking issues for other folks.

Would you mind firing up a draft PR with what you have so far, so I can see what you mean exactly and align on the implementation? This will prevent you from wasting time in a direction we don't want Mitosis to go into.

@Marvin-Brouwer
Copy link
Author

Sure, on it!

@Marvin-Brouwer
Copy link
Author

Here you go: #1336
I included some comments about the backwards compatibility thing I'd like to address if you decide to go forward with this.

@Marvin-Brouwer
Copy link
Author

I actually noticed that solid already has this feature.
BTW, if this is not the direction you'd like to go on.
Could this also be implemented as a plugin perhaps?
I really needs slots with data for what I'm doing

@Marvin-Brouwer
Copy link
Author

@samijaber
Any news?

@Marvin-Brouwer
Copy link
Author

@samijaber is there anything I can do to help move this forward?

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

No branches or pull requests

2 participants