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

Function-based Component API (prop definition sharing) #71

Open
backbone87 opened this issue Jul 8, 2019 · 8 comments
Open

Function-based Component API (prop definition sharing) #71

backbone87 opened this issue Jul 8, 2019 · 8 comments

Comments

@backbone87
Copy link

Personally i use mixins alot to share common props between components, for example:

@Component
export default class FlexChildStyle extends Vue {
  @LengthProp()
  public min?: number | string;
  @LengthProp()
  public max?: number | string;
  @LengthProp()
  public basis?: number | string;
  @IntegerProp(false, 0)
  public grow?: number;
  @IntegerProp(false, 0)
  public shrink?: number;

  public get flexChildStyle() {
    return {
      flexGrow: this.grow,
      flexShrink: this.shrink,
      flexBasis: toLength(this.basis),
      minWidth: toLength(this.min),
      maxWidth: toLength(this.max),
    };
  }
}

How would this be solved with the function API?

@skyrpex
Copy link

skyrpex commented Jul 8, 2019

The functional API won't help you with that AFAIK. You could share typescript definitions for the props, or export the props definitions:

// flex.js
export const FlexChildStyleProps = {
    min: { type: [Number, String], default: null },
    max: { type: [Number, String], default: null },
    basis: { type: [Number, String], default: null },
    grow: { type: Number, default: 0 },
    shrink: { type: Number, default: 0 },
};

// component.js
import { FlexChildStyleProps } from "./flex";

export default {
    props: {
        ...FlexChildStyleProps,
    },
};

@backbone87
Copy link
Author

hm, that would require to split this up into 2 parts (prop defs + useComputedFlexChildStyle).

i wonder if something like this would be possible:

export interface FlexChildStyleOptions {
  props?: {
    min?: string;
    max?: string;
    basis?: string;
    grow?: string;
    shrink?: string;
  };
}

export function useFlexChildStyle({ props = {} }: FlexChildStyleOptions) {
  const min = prop(props.min || 'min', { type: [Number, String], default: null });
  const max = prop(props.max || 'max', { type: [Number, String], default: null });
  const basis = prop(props.basis || 'basis', { type: [Number, String], default: null });
  const grow = prop(props.grow || 'grow', { type: Number, default: 0 });
  const shrink = prop(props.shrink || 'shrink', { type: Number, default: 0 });

  return computed(() => ({
    flexGrow: grow.value,
    flexShrink: shrink.value,
    flexBasis: toLength(basis.value),
    minWidth: toLength(min.value),
    maxWidth: toLength(max.value),
  }));
};

in this case, the prop function, creates a binding to the current comps properties. prop name is configurable to inverse naming control. this doesnt bind the prob value directly, but acts more like a computed, where the default value is applied (this allows props with same name in different useX with different default values). the rest of the prop config is for runtime validation and can be done during setup call.

then there is only the problem of introspection left, where a tool like storybook for example, wants to know about all props of a component, but this could be solvable by creating a dummy component (and therefore all prop calls can be collected).

@skyrpex
Copy link

skyrpex commented Jul 9, 2019

We can implement a user land prop helper to handle runtime type checking and default values, but I'm not sure if we can have props introspection without relying on the component props defintion.

// prop.ts
interface PropOptions<T> {
    type?: any;
    default?: T;
}

// Will handle runtime type checking and default values.
export const prop = <T>(getter: () => T, options: PropOptions<T>) => {
    return computed(() => {
        const value = getter();

        if (options.type) {
            // Check type
        }

        return value || options.default;
    })
};

// flexChildStyle.ts
export interface FlexChildStyleProps {
    min?: number | string,
    max?: number | string,
    basis?: number | string,
    grow?: number,
    shrink?: number,
};

export const useFlexChildStyle = (props: FlexChildStyleProps) => {
    const min = prop(() => props.min, { type: [Number, String], default: null });
    const max = prop(() => props.max, { type: [Number, String], default: null });
    const basis = prop(() => props.basis, { type: [Number, String], default: null });
    const grow = prop(() => props.grow, { type: Number, default: 0 });
    const shrink = prop(() => props.shrink, { type: Number, default: 0 });

    return computed(() => ({
        flexGrow: grow.value,
        flexShrink: shrink.value,
        flexBasis: toLength(basis.value),
        minWidth: toLength(min.value),
        maxWidth: toLength(max.value),
    }));
}

// component.ts
export default createComponent({
    setup(props: FlexChildStyleProps) {
        const flexChildStyle = useFlexChildStyle(props);
        return {
            flexChildStyle,
        };
    },
});

@aztalbot
Copy link

aztalbot commented Jul 9, 2019

I would expect to do it the way @skyrpex has it. That works well enough if the goal is simply sharing prop definitions. In the case of hooks and props, though, I would either define useFlexChildStyle.props and spread that, or export useFlexChildStyleProps and spread that. If using typescript I’d probably export the interface and extend the props interface of the component using the hook. I wonder though if we then end up defining our props twice with TS and props declaration (would a createHook helper be needed?).

@backbone87 I’m not sure I like the idea of a prop function. This is partly because props for that hook are then not tree-shakeable. I also feel it distracts from whatever other logic would go in that function, too. The prop function also doesn’t declare the prop on the component using the hook, meaning I can’t simply do useFlexChildStyle(props), I think I would have to do useFlexChildStyle(context.attrs) which seems weird and might not work if one of the props is declared while another isn’t (if I understand the optional props RFC correctly).

One thing I was hoping would be possible with props no longer being required is that they can be completely removed from the production build. Seeing as they are only really useful in development with runtime validation, this would make so much sense and stop us from shipping unused code in production. (Maybe that won’t be the default behavior though – it’s not like props amount to much code – but I think however we use props with hooks they should be opt-in and easily removable for production). (crossed this out after rereading the optional props rfc.)

Having thought about it a little now, I’m not sure I like that the functional API seems to completely punt on addressing props. Yes, they are just functions, but I suspect many of us will take the parameters to those functions and codify them as props in all components that use them. In that sense I think the function based API really should present more of an opinion on how to do this.

@backbone87
Copy link
Author

backbone87 commented Jul 9, 2019

i think you missunderstood how i envisioned the prop function. the first arg is not the prop value, but the prop name (though configurable, by passing the prop name as an arg to the useX function).

so the prop function cant be userland, since it "hooks" into the current components attrs. maybe exposing the prop function via setup context makes it more clear:

function useSize(prop, propName = 'size') {
  const size = prop(propName, { type: String });
  return computed(() => size.value === 'large' ? 100 : 10);
}

export default {
  setup(props, { prop }) {
    const size = useSize(prop, 'mySizeProp');
    // ...
  }
}

Yes, they are just functions, but I suspect many of us will take the parameters to those functions and codify them as props in all components that use them. In that sense I think the function based API really should present more of an opinion on how to do this.

exactly my thought

@skyrpex
Copy link

skyrpex commented Jul 9, 2019

Your approach has some problems:

  • It's unclear were these props are defined or declared. Could be anywhere.
  • A prop with the same name can be declared in different places, in different times and with different types.
  • It adds more methods to the core, and it's not clear enough why should one use it.

I still think that exporting prop definitions directly is much more simple and straightforward if you want to reuse them and have them available in tools:

// flex.js
export const FlexChildStyleProps = {
    min: { type: [Number, String], default: null },
    max: { type: [Number, String], default: null },
    basis: { type: [Number, String], default: null },
    grow: { type: Number, default: 0 },
    shrink: { type: Number, default: 0 },
};

// component.js
import { FlexChildStyleProps } from "./flex";

export default {
    props: {
        ...FlexChildStyleProps,
    },
};

@backbone87
Copy link
Author

It's unclear were these props are defined or declared. Could be anywhere.

This is in general the same problem as with mixins, but i am not of the opinion that we should avoid mixins at all costs. its just that the capabilities of mixins should be very specific and limited.

A prop with the same name can be declared in different places, in different times and with different types.

That is actually not really a problem, because the control of prop names can be inverted, so the setup function has control over the prop names (see my examples). Also it would be perfectly fine use-case if to different useX want to use the same prop. if the validation does not match, then this is a composition problem.

It adds more methods to the core, and it's not clear enough why should one use it.

The first part of the sentence is no argument, if the method has a clear use case. property definition sharing is definitely a common use case.


In general, i dont say that my approach is the perfect solution or even a good one. its just the one i came up with. your proposal is another approach, but it is more verbose, when i want to have some common computeds on these imported prop definitions (like the style computed in my example). and then i also have to match up the prop import with the setup's useX. for sure i could work with both approaches, but the reason for this issue is to present possibilities and carve out the do's and dont's of the function API.

@aztalbot
Copy link

aztalbot commented Jul 9, 2019

I did misunderstand. Seeing that example helped, thanks. It definitely seems to fit the design of the rest of the API.

I also think are right about props of the same name being a composition problem. But, the simplest solution, I think, is to have the component completely in control of props.

Also, what if I want to pass the hook a modified version of the prop (props.size * 2). I can’t seem to do that in your example because it’s picking the value straight out of attrs. You could have another parameter to provide that value, but now things are getting less clean. Or is there a better way?

The introduction of the prop function makes me feel like props should no longer be a separate parameter, too (it wouldn’t be as useful perhaps). Maybe that’s okay, because it could give us a way to declare props when doing createComponent, which is nice.

This is a good idea and I’m glad someone is trying to think through it. But I think we need to keep thinking on what the best and simplest solution will be.

My preference which I also don’t claim to be the right solution or pattern is to define .props on our hooks. That way when we import the hook we can decide whether we want to use the hooks props or not (you can even create modified versions of them if they don’t fit your needs). When using createComponent we can still define .props on the component same way we do functional components and just spread the hook props. That way the component has complete control over its interface and also how it passes data into hooks. I’m not opposed to a function like prop, this is just what first comes to mind when I think of how I would accomplish this in a simple and straightforward way. I don’t really see the downsides as it matches existing patterns. But let me know what isn’t ideal about that, just having it separate from the function? (that’s actually something I like :) )

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

3 participants