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

Feature request: Directives for components #96

Open
lxsmnsyc opened this issue Nov 10, 2021 · 6 comments
Open

Feature request: Directives for components #96

lxsmnsyc opened this issue Nov 10, 2021 · 6 comments

Comments

@lxsmnsyc
Copy link
Collaborator

lxsmnsyc commented Nov 10, 2021

Currently, the code <Counter use:example="Hello World" /> compiles into

createComponent(Counter, {
  "use:example": "Hello World"
})

We could probably make it work into

createComponent(Counter, {
  ref(node) {
    example(node, 'Hello World');
  },
});

since ref is already a special prop for both host and components.

What do you think?

The only design challenge would be that props.ref can be assigned anytime, anywhere which makes it lack constraint unlike host nodes, posing some memory leaks.

@ryansolid
Copy link
Owner

I think the other thing to solve is spreads if we want components to be able to forward these.

This proposed solution came up during 1.0 rc time period but ultimately wasn't a fan of the inconsistency. Mostly that it puts onus on the component author to forward ref/the right ref. These directives simply may or may not work depending, or not work as expected. Use could only apply to the main ref if multiple were forwarded. Expects precisely a DOM element. There are a lot of unexpected potential.

Svelte doesn’t support this and Im ok with not as well.

@edemaine
Copy link
Contributor

This issue came up again recently in #help, where it was surprising that <Dynamic component="div"> didn't support directives (or rather, they get assigned as attributes).

I wonder if it makes sense to handle directives when spreading them into Elements. (This may be what Ryan was suggesting.) Then a component could implement/override/whatever directives if they want, and can pass on other directives via spread.

At first I worried that this wouldn't play well with TypeScript. A component would seemingly need to specify/know every directive they could accept. But explicit enumeration can be avoided: we could say tyat a component can take arbitrary directives using the type `use:${string}` for keys in props. And we could build helper types for this, like DirectiveProps and DirectiveComponent that could get &ed on with other types.

@lxsmnsyc
Copy link
Collaborator Author

lxsmnsyc commented May 18, 2022

I think part of the issue with <Dynamic> is that if you do <Dynamic component={props.as} use:myDirective />, how would that be interpreted? Seems to me that it would pose an undefined behavior.

@edemaine
Copy link
Contributor

I had the following behavior in mind:

  1. If props.as is a native element, it would work like a regular directive.
  2. If props.as is a component, use:myDirective (=true?) would get passed in as a prop. The component could then do what it wants with the directive, including passing it down via spread to a native element or other component.

This is very close to the current behavior, which just does case 2 in all cases. I'm proposing changing the behavior when you get a native element.

I'm not exactly sure how ergonomic the DX would be here, but this change to spread would at least "fix" Dynamic when component is a native element, which seems like a nice step.

@fabiospampinato
Copy link

fabiospampinato commented Jul 10, 2022

IMO a workable solution for this is the following:

  1. Custom components expose a custom prop for receiving regular refs.
  2. Custom components attach those refs to a native element of their choosing.
  3. Ref arrays are supported.
  4. Directives are more structured and can be turned into ref functions via something like MyDirective.ref ( arg1, arg2 ).
  5. The user simply gets refs for the directives that it needs and passes them on to the component, which attaches them to the native element. (which as a side effect is also potentially more convenient to write for conditional directives)

Reasoning:

  • 1. doesn't introduce any new concepts, regular refs are just something that a custom component may want to support.
  • 2. probably has to happen because how would the framework attach these itself? It won't make sense for every custom component to support directives, and which element to target may not be obvious, or a custom component may expose ways to attach refs to multiple elements, this isn't something that the framework can figure out automatically.
  • Without 3. I don't see a clean way for the custom component to attach those refs, making a merged ref manually seems just verbose for no reason. This would also be a way for native elements to support this pattern out of the box, for consistency.
  • Without 4. or similar one can't turn a directive into a ref, though this requires changing or extending how directives work.

Basically if being able to attach directives to different elements inside a single custom component is desirable I don't see any other option. If that's not desirable it's probably possible to introduce like a special directives prop which gets populated with the directives the user passed on, and then maybe the custom component can say if it supports directives or not by providing a type for props.directives or not 🤔

@edemaine
Copy link
Contributor

edemaine commented Jul 10, 2022

First, whatever we decide here in general, even if it's "do nothing", I think we should modify <Dynamic> to support directives when it component is a string so an Element gets created. Otherwise, every library that uses <Dynamic> in this way (e.g. styled components, MDX, ...) would need to do this themselves, which seems like gross repetition.

Now, reflecting on this issue more, I'm now much more inclined to go with @lxsmnsyc's original suggestion (instead of modifying spread):

use:foo={bar} is always just syntactic sugar for ref={(r) => foo(r, bar)} (plus automatic merging of such refs)

I think this is nicely consistent, and easy to teach and understand. The current docs say
(and have said for a long time) "In a sense this [use:___] is just syntax sugar over ref but allows us to easily attach multiple directives to a single element."

The arguments against this are that "ref could get passed anything! and at any time!" But this is a general issue with ref, and in practice props.ref is often just passed on to a relevant DOM element. We still find ref useful in many situations with components. I think we will find use:foo to also be useful in many situations with components, whereas the current behavior is basically never useful, because with it the component needs to know about directives.

With this proposal, the user of a directive (whoever writes <Comp use:foo/>) needs to know how <Comp ref={ref}> will set ref, and what foo will do with that as an argument. This seems consistent with how directives work for Elements now. I don't expect to be able to call function foo with any argument, only those that it's designed to work for. If I know Comp will give me such a thing via ref, then I can use use:foo. Currently one has to write ref={foo} or ref={(r) => foo(r, bar)}, which gets especially messy with multiple refs. The whole point of use: is to provide this syntactic sugar.

On the TypeScript side, one issue is that interface DirectiveFunctions would need to change, so that the first argument is unknown instead of Element. But the whole point of DirectiveFunctions is for the user to override it to specify the correct types for the directive, so this makes sense.

interface DirectiveFunctions {
[x: string]: (el: Element, accessor: Accessor<any>) => void;
}

Hopefully we can extend DirectiveFunctionAttributes<T> to properly detect a component with props.ref types, and use the argument type there to specify what the directive will be given as first argument.

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

4 participants