BuilderIO / mitosis

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

Svelte: replace `props` with `$$props` or `$$restProps` #1070

Closed codyebberson closed 1 year ago

codyebberson commented 1 year ago

I am interested in helping provide a fix!

Yes

Which generators are impacted?

Reproduction case

https://mitosis.builder.io/?outputTab=E4UwhgxgLkA%3D&code=JYWwDg9gTgLgBAbzgVwM4FMDKMCGN1wC%2BcAZlBCHAOQACARssADYAm6UAdMBAPQjAwIqYKioBuAFChIsRHAbMWAYSY5UqAHI4QBYmQrUOfAJ4BaZDGajJE9AA8Z8NiRzIm8EsgB2AY0sQvOABZYyUKSC90LxgACjByMFQASkQJODgodBhkKECAHhZgADc4H1V1AF4EBVYVNU1tdDiE5MIAPgAxCAg8nkKitslCCSA%3D%3D%3D

Expected Behaviour

First, I'm not sure this use case matches how you guys think about Mitosis props. If I'm doing something horribly wrong, please feel free to tell me so.

Consider this input:

import { buildClassName } from './my-utils';

export default function MyComponent(props) {
  return <div class={buildClassName(props)}>{props.children}</div>;
}

Note the call to buildClassName() that takes the whole props object. The idea is that there would be common props for many components, and a general purpose buildClassName() utility would parse those props.

That works great with React:

import * as React from "react";
import { buildClassName } from "./my-utils";

function MyComponent(props) {
  return <div className={buildClassName(props)}>{props.children}</div>;
}

export default MyComponent;

That works great with Solid:

import { buildClassName } from "./my-utils";

function MyComponent(props) {
  return <div class={buildClassName(props)}>{props.children}</div>;
}

export default MyComponent;

In Svelte, you get the following output:

<script>
  import { buildClassName } from "./my-utils";
</script>

<div class={buildClassName(props)}><slot /></div>

That does not work as is. However, it would work if buildClassName(props) were replaced with buildClassName($$props) or buildClassName($$restProps).

It seems that most (all?) frameworks have some mechanism for "get all props" or "get all component attributes", and that this pattern should be possible for all output targets.

The ability to use all props is essential for a component library where you have some notion of utility features on many components.

Actual Behaviour

See above

Additional Information

I started exploring how to fix. It seems like this should be in _replaceIdentifiers in packages\core\src\helpers\replace-identifiers.ts. If you're ok with it, I'd be happy to take this on.

Thanks so much for all your work on this project. I love what you're doing.

samijaber commented 1 year ago

You can definitely fix this! props value access should be replaced with $$props in Svelte.

Worth noting that passing the entire props around is suboptimal perf-wise in Svelte, and I think also in some the other frameworks that have compilers (I can't remember which exactly):

CleanShot 2023-04-03 at 17 49 58@2x

So I'd personally suggest moving away from passing the entire props to utility functions. But yeah, the code should definitely be fixed for Svelte!

samijaber commented 1 year ago

To fix it, this is the function that processes all props and state identifiers:

https://github.com/BuilderIO/mitosis/blob/8b4f9b7085af7457041333157a1cb1bf078a3b22/packages/core/src/generators/svelte/helpers.ts#L6-L17

I think we can use a pipe to add another transformation:

import { MitosisComponent } from '../../types/mitosis-component';
import { stripStateAndPropsRefs } from '../../helpers/strip-state-and-props-refs';
import { isSlotProperty, replaceSlotsInString } from '../../helpers/slots';
import { ToSvelteOptions } from './types';
import { pipe } from 'fp-ts/lib/function';
import { replaceIdentifiers } from 'src/helpers/replace-identifiers';

export const stripStateAndProps =
  ({ options, json }: { options: ToSvelteOptions; json: MitosisComponent }) =>
  (code: string) =>
    pipe(
      stripStateAndPropsRefs(code, {
        includeState: options.stateType === 'variables',
        replaceWith: (name) =>
          name === 'children'
            ? '$$slots.default'
            : isSlotProperty(name)
            ? replaceSlotsInString(name, (x) => `$$slots.${x}`)
            : name,
      }),
      (x) =>
        replaceIdentifiers({ code: x, from: 'props', to: '$$props' }),
    );

I think that should work 🤞🏽. Can never fully remember how the to param works for replaceIdentifiers 😅, so you'll have to try and see.

codyebberson commented 1 year ago

Thanks @samijaber - it turns out there is no way to implement "get all props" in Angular. That, combined with the perf warning in Svelte, made me reconsider this strategy.

For what it's worth, I implemented your suggestion, and it works exactly as advertised. https://github.com/codyebberson/mitosis/commits/cody-svelte-all-props

But seeing how it breaks most frameworks, I feel like it's a bad idea.

Thanks agin