ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-dom/src/__tests__/ReactDOMServerIntegrationLegacyContextDisabled-test.internal.js (JavaScript)

Code before:
        lifecycleContextLog.push(nextContext);
      }
      render() {
        return formatValue(this.context);
      }
    }

    function LegacyFnConsumer(props, context) {
      return formatValue(context);
    }
    LegacyFnConsumer.contextTypes = {foo() {}};

    function RegularFn(props, context) {
      return formatValue(context);
    }

    const e = await render(
      <LegacyProvider>
        <span>
          <LegacyClsConsumer />
          <LegacyFnConsumer />
          <RegularFn />
        </span>
      </LegacyProvider>,
      3,
    );
    expect(e.textContent).toBe('{}undefinedundefined');
    expect(lifecycleContextLog).toEqual([]);
  });

  itRenders('modern context', async render => {
    const Ctx = React.createContext();

    class Provider extends React.Component {
      render() {
        return (
          <Ctx.Provider value={this.props.value}>
            {this.props.children}
          </Ctx.Provider>
        );
      }
    }

    class RenderPropConsumer extends React.Component {
      render() {
        return <Ctx.Consumer>{value => formatValue(value)}</Ctx.Consumer>;
      }
    }

    const lifecycleContextLog = [];
    class ContextTypeConsumer extends React.Component {

Review comment (suggestion):
In the new root we retry with client rendering on hydration mismatch. Since the warning isn't deduped, we get twice the errors.

With https://github.com/facebook/react/pull/30299 the warning is going to be deduped.

Code after:
      UNSAFE_componentWillUpdate(nextProps, nextState, nextContext) {
        lifecycleContextLog.push(nextContext);
      }
      render() {
        return formatValue(this.context);
      }
    }

    function LegacyFnConsumer(props, context) {
      return formatValue(context);
    }
    LegacyFnConsumer.contextTypes = {foo() {}};

    function RegularFn(props, context) {
      return formatValue(context);
    }

    const e = await render(
      <LegacyProvider>
        <span>
          <LegacyClsConsumer />
          <LegacyFnConsumer />
          <RegularFn />
        </span>
      </LegacyProvider>,
      render === clientRenderOnBadMarkup ? 6 : 3,
    );
    expect(e.textContent).toBe('{}undefinedundefined');
    expect(lifecycleContextLog).toEqual([]);
  });

  itRenders('modern context', async render => {
    const Ctx = React.createContext();

    class Provider extends React.Component {
      render() {
        return (
          <Ctx.Provider value={this.props.value}>
            {this.props.children}
          </Ctx.Provider>
        );
      }
    }

    class RenderPropConsumer extends React.Component {
      render() {
        return <Ctx.Consumer>{value => formatValue(value)}</Ctx.Consumer>;
      }
    }

    const lifecycleContextLog = [];
Repository: facebook/react
File: scripts/flow/xplat.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

// eslint-disable-next-line no-unused-vars
declare const RN$Bridgeless: boolean;

declare module 'ReactNativeInternalFeatureFlags' {
  declare export var alwaysThrottleRetries: boolean;
  declare export var enableComponentStackLocations: boolean;
  declare export var enableDeferRootSchedulingToMicrotask: boolean;
  declare export var enableUseRefAccessWarning: boolean;
  declare export var passChildrenWhenCloningPersistedNodes: boolean;
  declare export var useMicrotasksForSchedulingInFabric: boolean;
  declare export var enableUnifiedSyncLane: boolean;
}


Review comment (suggestion):
Can you put this in `react-native-host-hooks.js` to match `nativeFabricUIManager` instead? The xplat.js flow file is for internal-only modules but this global will be read in open source builds.

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

declare module 'ReactNativeInternalFeatureFlags' {
  declare export var alwaysThrottleRetries: boolean;
  declare export var consoleManagedByDevToolsDuringStrictMode: boolean;
  declare export var enableAsyncActions: boolean;
  declare export var enableComponentStackLocations: boolean;
  declare export var enableDeferRootSchedulingToMicrotask: boolean;
  declare export var enableInfiniteRenderLoopDetection: boolean;
  declare export var enableRenderableContext: boolean;
  declare export var enableUnifiedSyncLane: boolean;
  declare export var enableUseRefAccessWarning: boolean;
  declare export var passChildrenWhenCloningPersistedNodes: boolean;
  declare export var useModernStrictMode: boolean;
}
Repository: facebook/react
File: .eslintrc.js (JavaScript)

Code before:
    SyntheticEvent: 'readonly',
    SyntheticMouseEvent: 'readonly',
    Thenable: 'readonly',
    TimeoutID: 'readonly',
    WheelEventHandler: 'readonly',
    FinalizationRegistry: 'readonly',

    spyOnDev: 'readonly',
    spyOnDevAndProd: 'readonly',
    spyOnProd: 'readonly',
    __DEV__: 'readonly',
    __EXPERIMENTAL__: 'readonly',
    __EXTENSION__: 'readonly',
    __PROFILE__: 'readonly',
    __TEST__: 'readonly',
    __UMD__: 'readonly',
    __VARIANT__: 'readonly',
    __unmockReact: 'readonly',
    gate: 'readonly',
    trustedTypes: 'readonly',
    IS_REACT_ACT_ENVIRONMENT: 'readonly',
    AsyncLocalStorage: 'readonly',
    async_hooks: 'readonly',
    globalThis: 'readonly',

    RN$Bridgeless: 'readonly',
  },
};


Review comment (refactor):
Can you move this up to specify a specific file like `nativeFabricUIManager` does above? 

https://github.com/facebook/react/blob/main/.eslintrc.js#L419-L424

Code after:
    React$Context: 'readonly',
    React$Element: 'readonly',
    React$ElementConfig: 'readonly',
    React$ElementProps: 'readonly',
    React$ElementRef: 'readonly',
    React$ElementType: 'readonly',
    React$Key: 'readonly',
    React$Node: 'readonly',
    React$Portal: 'readonly',
    React$Ref: 'readonly',
    ReadableStreamController: 'readonly',
    RequestInfo: 'readonly',
    RequestOptions: 'readonly',
    StoreAsGlobal: 'readonly',
    symbol: 'readonly',
    SyntheticEvent: 'readonly',
    SyntheticMouseEvent: 'readonly',
    Thenable: 'readonly',
    TimeoutID: 'readonly',
    WheelEventHandler: 'readonly',
    FinalizationRegistry: 'readonly',

    spyOnDev: 'readonly',
    spyOnDevAndProd: 'readonly',
    spyOnProd: 'readonly',
    __DEV__: 'readonly',
    __EXPERIMENTAL__: 'readonly',
    __EXTENSION__: 'readonly',
    __PROFILE__: 'readonly',
    __TEST__: 'readonly',
    __UMD__: 'readonly',
    __VARIANT__: 'readonly',
    __unmockReact: 'readonly',
    gate: 'readonly',
    trustedTypes: 'readonly',
    IS_REACT_ACT_ENVIRONMENT: 'readonly',
    AsyncLocalStorage: 'readonly',
    async_hooks: 'readonly',
    globalThis: 'readonly',
  },
};
Repository: facebook/react
File: packages/react-test-renderer/npm/shallow.js (JavaScript)

Code before:
'use strict';

function ReactShallowRenderer() {
  throw new Error(
    'react-test-renderer is deprecated. To use react-test-renderer/shallow, import react-shallow-renderer directly. See https://react.dev/warnings/react-test-renderer'
  );
}

module.exports = ReactShallowRenderer;


Review comment (bug):
Nit: "deprecated" usually doesn't mean "removed" but in this case, this is removed. I would say something like this instead:

> react-test-renderer/shallow has been removed. See https://react.dev/warnings/react-test-renderer.

I'm not sure we should direct to react-shallow-renderer in the error message, or if we should mention it on the warning page instead. Really, people shouldn't be using shallow rendering at all, and direct people to that package would lead them in the wrong direction. 

Code after:
'use strict';

function ReactShallowRenderer() {
  throw new Error(
    'react-test-renderer/shallow has been removed. See https://react.dev/warnings/react-test-renderer.'
  );
}

module.exports = ReactShallowRenderer;
Repository: facebook/react
File: packages/react/src/ReactCurrentBatchConfig.js (JavaScript)

Code before:
 *
 * @flow
 */

import type {BatchConfigTransition} from 'react-reconciler/src/ReactFiberTracingMarkerComponent';
import type {
  EventPriority,
  NoEventPriorityType,
} from 'react-reconciler/src/ReactEventPriorities';

type BatchConfig = {
  transition: BatchConfigTransition | null,
  eventPriority: EventPriority,
};
/**
 * Keeps track of the current batch's configuration such as how long an update
 * should suspend for if it needs to.
 */
const ReactCurrentBatchConfig: BatchConfig = {
  transition: null,
  // We can't directly import from reconciler but we want to ensure
  // this default value matches up with the expected event priority
  // for NoEventPriority so instead we assert the type and then
  // cast to avoid the opaque type restriction
  // $FlowFixMe[incompatible-type]: NoEventPriority is valid but EventPriority is opaque
  eventPriority: (0: NoEventPriorityType),
};

export default ReactCurrentBatchConfig;


Review comment (performance):
Event priority is a very Fiber centric concept where as this store is intended to be more generic.

Originally `BatchConfigTransition.transition` was supposed to be just the options passed in to the public API but now it has become this very Fiber centric thing even importing the type from Fiber.

Ideally it would be more generic so that it can be implemented in another implementation too.

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import type {BatchConfigTransition} from 'react-reconciler/src/ReactFiberTracingMarkerComponent';

export type BatchConfig = {
  transition: BatchConfigTransition | null,
};
/**
 * Keeps track of the current batch's configuration such as how long an update
 * should suspend for if it needs to.
 */
const ReactCurrentBatchConfig: BatchConfig = {
  transition: null,
};

export default ReactCurrentBatchConfig;
Repository: facebook/react
File: packages/react-reconciler/src/ReactFiberHooks.js (JavaScript)

Code before:
  initialState: Awaited<S>,
  permalink?: string,
): [Awaited<S>, (P) => void, boolean] {
  // Unlike useState, useFormState doesn't support render phase updates.
  // Also unlike useState, we need to replay all pending updates again in case
  // the passthrough value changed.
  //
  // So instead of a forked re-render implementation that knows how to handle
  // render phase udpates, we can use the same implementation as during a
  // regular mount or update.
  const stateHook = updateWorkInProgressHook();
  const currentStateHook = currentHook;

  if (currentStateHook !== null) {
    // This is an update. Process the update queue.
    return updateFormStateImpl(
      stateHook,
      currentStateHook,
      action,
      initialState,
      permalink,
    );
  }

  // For mount, pending is always false.
  const [isPending] = rerenderState(false);

  // This is a mount. No updates to process.
  const state: Awaited<S> = stateHook.memoizedState;

  const actionQueueHook = updateWorkInProgressHook();
  const actionQueue = actionQueueHook.queue;
  const dispatch = actionQueue.dispatch;

  // This may have changed during the rerender.
  actionQueueHook.memoizedState = action;

  return [state, dispatch, isPending];
}

function pushEffect(
  tag: HookFlags,
  create: () => (() => void) | void,
  inst: EffectInstance,
  deps: Array<mixed> | null,
): Effect {
  const effect: Effect = {
    tag,
    create,
    inst,
    deps,

Review comment (suggestion):
Since we don't actually need the result, all we need to do is advance the hook list.

```suggestion
  updateWorkInProgressHook();
```

Code after:
  initialState: Awaited<S>,
  permalink?: string,
): [Awaited<S>, (P) => void, boolean] {
  // Unlike useState, useFormState doesn't support render phase updates.
  // Also unlike useState, we need to replay all pending updates again in case
  // the passthrough value changed.
  //
  // So instead of a forked re-render implementation that knows how to handle
  // render phase udpates, we can use the same implementation as during a
  // regular mount or update.
  const stateHook = updateWorkInProgressHook();
  const currentStateHook = currentHook;

  if (currentStateHook !== null) {
    // This is an update. Process the update queue.
    return updateFormStateImpl(
      stateHook,
      currentStateHook,
      action,
      initialState,
      permalink,
    );
  }

  updateWorkInProgressHook(); // State

  // This is a mount. No updates to process.
  const state: Awaited<S> = stateHook.memoizedState;

  const actionQueueHook = updateWorkInProgressHook();
  const actionQueue = actionQueueHook.queue;
  const dispatch = actionQueue.dispatch;

  // This may have changed during the rerender.
  actionQueueHook.memoizedState = action;

  // For mount, pending is always false.
  return [state, dispatch, false];
}

function pushEffect(
  tag: HookFlags,
  create: () => (() => void) | void,
  inst: EffectInstance,
  deps: Array<mixed> | null,
): Effect {
  const effect: Effect = {
    tag,
    create,
    inst,
    deps,
Repository: facebook/react
File: packages/react-dom/src/__tests__/ReactTestUtils-test.js (JavaScript)

Code before:
    ReactTestUtils.renderIntoDocument(<Component />);
    expect(mockArgs.length).toEqual(0);
  });

  // @gate !disableDOMTestUtils
  it('should find rendered component with type in document', () => {
    class MyComponent extends React.Component {
      render() {
        return true;
      }
    }

    const instance = ReactTestUtils.renderIntoDocument(<MyComponent />);
    const renderedComponentType = ReactTestUtils.findRenderedComponentWithType(
      instance,
      MyComponent,
    );

    expect(renderedComponentType).toBe(instance);
  });

  // @gate disableDOMTestUtils
  it('throws on every removed function with a special message', async () => {
    expect(ReactTestUtils.isDOMComponent).toThrowError(
      '`isDOMComponent` was removed from `react-dom/test-utils`. ' +
        'For testing React, we recommend React Testing Library instead: https://testing-library.com/docs/react-testing-library/intro.',
    );
  });

  // @gate __DEV__
  it('warns when using `act`', () => {
    expect(() => {
      ReactTestUtils.act(() => {});
    }).toErrorDev(
      [
        '`ReactDOMTestUtils.act` is deprecated in favor of `React.act`. ' +
          'Import `act` from `react` instead of `react-dom/test-utils`.',
      ],
      {withoutStack: true},
    );
  });
});


Review comment (suggestion):
Similar to the react-test-renderer warning, I don't think we should hard code the recommendations into the error message, instead something like:

> `isDOMComponent` was removed from `react-dom/test-utils`. See https://react.dev/warnings/react-test-renderer for more info.

Code after:
      });

      const input = ReactTestUtils.findRenderedDOMComponentWithTag(
        instance,
        'input',
      );
      input.value = 'giraffe';
      ReactTestUtils.Simulate.change(input);

      expect(onChange).toHaveBeenCalledWith(
        expect.objectContaining({target: input}),
      );
    });

    // @gate !disableDOMTestUtils
    it('should have mouse enter simulated by test utils', async () => {
      const idCallOrder = [];
      const recordID = function (id) {
        idCallOrder.push(id);
      };
      let CHILD;
      function Child(props) {
        return (
          <div
            ref={current => (CHILD = current)}
            onMouseEnter={() => {
              recordID(CHILD);
            }}
          />
        );
      }

      class ChildWrapper extends React.PureComponent {
        render() {
          return <Child />;
        }
      }

      const container = document.createElement('div');
      const root = ReactDOMClient.createRoot(container);
      await act(() => {
        root.render(
          <div>
            <div>
              <ChildWrapper />
              <button disabled={true} />
            </div>
          </div>,
        );
      });
      await act(() => {
Repository: facebook/react
File: packages/react-client/src/ReactFlightClient.js (JavaScript)

Code before:
}

function createModelResolver<T>(
  chunk: SomeChunk<T>,
  parentObject: Object,
  key: string,
  cyclic: boolean,
): (value: any) => void {
  let blocked;
  if (initializingChunkBlockedModel) {
    blocked = initializingChunkBlockedModel;
    if (!cyclic) {
      blocked.deps++;
    }
  } else {
    blocked = initializingChunkBlockedModel = {
      deps: cyclic ? 0 : 1,
      value: (null: any),
    };
  }
  return value => {
    parentObject[key] = value;

    // If this is a tuple that's representing a react element, and the props
    // were resolved (4th item, key '3'), the props of `blocked.value` must be
    // updated.
    if (
      Array.isArray(parentObject) &&
      key === '3' &&
      blocked.value &&
      blocked.value.$$typeof === REACT_ELEMENT_TYPE
    ) {
      blocked.value.props = value;
    }

    // If this is the root object for a model reference, where `blocked.value`
    // is a stale `null`, the resolved value can be used directly.
    if (key === '' && blocked.value === null) {
      blocked.value = value;
    }

    blocked.deps--;
    if (blocked.deps === 0) {
      if (chunk.status !== BLOCKED) {
        return;
      }
      const resolveListeners = chunk.value;
      const initializedChunk: InitializedChunk<T> = (chunk: any);
      initializedChunk.status = INITIALIZED;
      initializedChunk.value = blocked.value;
      if (resolveListeners !== null) {

Review comment (suggestion):
This is clever. I think this hold true because if the root is blocked, there can't be any other objects that are also blocked. At least for now. Might not hold in the future but should be ok for now at least.

Code after:
}

function createModelResolver<T>(
  chunk: SomeChunk<T>,
  parentObject: Object,
  key: string,
  cyclic: boolean,
): (value: any) => void {
  let blocked;
  if (initializingChunkBlockedModel) {
    blocked = initializingChunkBlockedModel;
    if (!cyclic) {
      blocked.deps++;
    }
  } else {
    blocked = initializingChunkBlockedModel = {
      deps: cyclic ? 0 : 1,
      value: (null: any),
    };
  }
  return value => {
    parentObject[key] = value;

    // If this is the root object for a model reference, where `blocked.value`
    // is a stale `null`, the resolved value can be used directly.
    if (key === '' && blocked.value === null) {
      blocked.value = value;
    }

    blocked.deps--;
    if (blocked.deps === 0) {
      if (chunk.status !== BLOCKED) {
        return;
      }
      const resolveListeners = chunk.value;
      const initializedChunk: InitializedChunk<T> = (chunk: any);
      initializedChunk.status = INITIALIZED;
      initializedChunk.value = blocked.value;
      if (resolveListeners !== null) {
        wakeChunk(resolveListeners, blocked.value);
      }
    }
  };
}

function createModelReject<T>(chunk: SomeChunk<T>): (error: mixed) => void {
  return (error: mixed) => triggerErrorOnChunk(chunk, error);
}

function createServerReferenceProxy<A: Iterable<any>, T>(
  response: Response,
Repository: facebook/react
File: packages/react/src/__tests__/ReactElementClone-test.js (JavaScript)

Code before:

  it('should extract null key and ref', () => {
    const element = React.createElement(ComponentClass, {
      key: '12',
      ref: '34',
      foo: '56',
    });
    const props = {
      key: null,
      ref: null,
      foo: 'ef',
    };
    const clone = React.cloneElement(element, props);
    expect(clone.type).toBe(ComponentClass);
    expect(clone.key).toBe('null');
    if (gate(flags => flags.enableRefAsProp)) {
      expect(clone.ref).toBe(null);
      expect(clone.props).toEqual({foo: 'ef', ref: null});
    } else {
      expect(clone.ref).toBe(null);
      expect(clone.props).toEqual({foo: 'ef'});
    }

    if (__DEV__) {
      expect(Object.isFrozen(element)).toBe(true);
      expect(Object.isFrozen(element.props)).toBe(true);
    }
  });

  it('throws an error if passed null', () => {
    const element = null;
    expect(() => React.cloneElement(element)).toThrow(
      'The argument must be a React element, but you passed null.',
    );
  });

  it('throws an error if passed undefined', () => {
    let element;
    expect(() => React.cloneElement(element)).toThrow(
      'The argument must be a React element, but you passed undefined.',
    );
  });
});


Review comment (suggestion):
@josephsavona since `enableRefAsProp` is on, the props will include the `ref`, so I updated the test.

Code after:
      foo: 'ef',
    };
    const clone = React.cloneElement(element, props);
    expect(clone.type).toBe(ComponentClass);
    expect(clone.key).toBe('12');
    if (gate(flags => flags.enableRefAsProp && flags.disableStringRefs)) {
      expect(clone.props.ref).toBe('34');
      expect(() => expect(clone.ref).toBe('34')).toErrorDev(
        'Accessing element.ref was removed in React 19',
        {withoutStack: true},
      );
      expect(clone.props).toEqual({foo: 'ef', ref: '34'});
    } else if (
      gate(flags => !flags.enableRefAsProp && !flags.disableStringRefs)
    ) {
      expect(clone.ref).toBe(element.ref);
      expect(clone.props).toEqual({foo: 'ef'});
    } else if (
      gate(flags => flags.enableRefAsProp && !flags.disableStringRefs)
    ) {
      expect(() => {
        expect(clone.ref).toBe(element.ref);
      }).toErrorDev('Accessing element.ref was removed in React 19', {
        withoutStack: true,
      });
      expect(clone.props).toEqual({foo: 'ef', ref: element.ref});
    } else {
      // Not going to bother testing every possible combination.
    }
    if (__DEV__) {
      expect(Object.isFrozen(element)).toBe(true);
      expect(Object.isFrozen(element.props)).toBe(true);
    }
  });

  it('should extract null key and ref', () => {
    const element = React.createElement(ComponentClass, {
      key: '12',
      ref: '34',
      foo: '56',
    });
    const props = {
      key: null,
      ref: null,
      foo: 'ef',
    };
    const clone = React.cloneElement(element, props);
    expect(clone.type).toBe(ComponentClass);
    expect(clone.key).toBe('null');
    if (gate(flags => flags.enableRefAsProp)) {
      expect(clone.ref).toBe(null);