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

Pass ref as normal prop #28348

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Pass ref as normal prop #28348

merged 4 commits into from
Feb 20, 2024

Commits on Feb 20, 2024

  1. Combine createElement and JSX modules

    There's a ton of overlap between the createElement implementation and
    the JSX implementation, so I combined them into a single module.
    
    In the actual build output, the shared code between JSX and
    createElement will get duplicated anyway, because react/jsx-runtime and
    react (where createElement livs) are separate, flat build artifacts.
    
    So this is more about code organization — with a few key exceptions,
    the implementations of createElement and jsx are highly coupled.
    acdlite committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    a1af97b View commit details
    Browse the repository at this point in the history
  2. Add enableRefAsProp feature flag

    Adding a flag for this so it can be rolled out incrementally at Meta.
    acdlite committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    7b636d2 View commit details
    Browse the repository at this point in the history
  3. Pass ref as normal prop

    Changes the behavior of the JSX runtime to pass through `ref` as a
    normal prop, rather than plucking it from the props object and storing
    on the element.
    
    This is a breaking change since it changes the type receiving component.
    However, most code is unaffected since it's unlikely that a component
    would have attempted to access a `ref` prop, since it was not possible
    to get a reference to one.
    
    `forwardRef` _will_ still pluck `ref` from the props object, though,
    since it's extremely common for users to spread the props object onto
    the inner component and pass `ref` as a differently named prop. This is
    for maximum compatibility with existing code — the real impact of this
    change is that `forwardRef` is no longer required.
    
    Currently, refs are resolved during child reconciliation and stored on
    the fiber. As a result of this change, we can move ref resolution to
    happen only much later, and only for components that actually use them.
    Then we can remove the `ref` field from the Fiber type. I have not yet
    done that in this step, though.
    acdlite committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    c46fc90 View commit details
    Browse the repository at this point in the history
  4. Keep element.ref for now but warn on access

    In the last commit I removed the `ref` property from the element type
    completely. Instead, let's keep it for another release cycle but warn
    if it's accessed.
    
    In dev, we add a non-enumerable getter with `defineProperty` and warn
    whenever it's invoked.
    
    We don't warn on access if a ref is not given. This reduces false
    positives in cases where a test serializer uses
    `getOwnPropertyDescriptors`` to compare objects, like Jest does, which
    is a problem because it bypasses non-enumerability.
    
    So unfortunately this will trigger a false positive warning in Jest when
    the diff is printed:
    
      expect(<div ref={ref} />).toEqual(<span ref={ref} />);
    
    A bit sketchy, but this is what we've done for the `props.key` and
    `props.ref` accessors for years, which implies it will be good enough
    for `element.ref`, too. Let's see if anyone complains.
    acdlite committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    63c9fde View commit details
    Browse the repository at this point in the history