bikeshaving / crank

The Just JavaScript Framework
https://crank.js.org
MIT License
2.7k stars 75 forks source link

Make `ref` callbacks fire once per element, and only for host elements #272

Closed brainkim closed 5 months ago

brainkim commented 10 months ago

I’ve been thinking about the ref property and would like to make the following breaking changes. The code which implements refs can be found in the core Crank code, and the current behavior of the ref prop can be described as follows:

I find both of these behaviors problematic. In the case of host elements, which represent singular DOM nodes, the value associated with an element will never change due to the virtual DOM algorithm, so firing the ref callback for each render is unnecessary. Furthermore, this could potentially be misused as an ad-hoc lifecycle. Taking a step back, the main purpose of refs is to create references to DOM nodes without having to resort to querying the DOM, and actual lifecycle logic should be done in schedule() or flush() callbacks.

In the case of component elements, the situation is even more problematic. While the fire-every-render behavior makes sense for component elements, because a component’s associated value is dynamic, the values which can be passed to ref callbacks are wildly polymorphic (string, DOM node, array of strings and DOM nodes, null). This means that the element creator needs an intimate understanding of what the component yields/returns, thereby violating the component as an abstraction. Forcing the component consumer to understand exactly what a component yields/returns can lead to brittle code.


To fix this situation, I propose the following API changes:

  1. For host elements, the ref callback should only be called when an element is created.
  2. For component elements, the ref callback will by default do nothing. Components themselves will be responsible for handling the ref similar to React.forwardRef() if they want to expose a DOM node to parents.
    
    function *Component() {
    /* ... */
    for ({} of this) {
    yield <Input $ref={ref} />
    }
    }

function Input({ref}) { return <input $ref={ref} /> }



Whether the component reads the `$ref` from props, or reads the `ref` from a new context method (`this.ref` getter maybe) is something to think about. Currently, the `$ref` property is deleted from the props object, along with `$key` and `$static`. See https://github.com/bikeshaving/crank/issues/259#issuecomment-1871800207 for related discussion about the special props being deleted from the component.

This issue depends a little bit on https://github.com/bikeshaving/crank/issues/259.
brainkim commented 5 months ago

Implemented in 0.6