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

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

Closed
brainkim opened this issue Jan 9, 2024 · 1 comment
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@brainkim
Copy link
Member

brainkim commented Jan 9, 2024

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:

  • If a ref callback is found on a host element, it is called every time an element is rendered with the value of the underlying element, e.g. the DOM node related to each element in the case of the DOMRenderer.

  • The ref callback on a component element will be called every time the component is rendered with the component element’s associated value, same as a host element. However, unlike host element, which have a 1-to-1 relationship with the underlying DOM node, a component element can be associated with 0, 1 or many values depending on what the component yields/returns.

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 #259 (comment) for related discussion about the special props being deleted from the component.

This issue depends a little bit on #259.

@brainkim brainkim self-assigned this Jan 9, 2024
@brainkim brainkim added the enhancement New feature or request label Jan 9, 2024
@brainkim brainkim added this to the 0.6 milestone Jan 9, 2024
@brainkim brainkim changed the title Make ref callbacks fire once per element Make ref callbacks fire once per element, and only for host elements Jan 10, 2024
@brainkim
Copy link
Member Author

Implemented in 0.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant