ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
887 stars 127 forks source link

Feature request: Directives for components #96

Open lxsmnsyc opened 3 years ago

lxsmnsyc commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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.

https://github.com/ryansolid/dom-expressions/blob/df0486922723a20a2b346d08af3c5ce1367489ed/packages/dom-expressions/src/jsx.d.ts#L63-L65

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.