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

Set owner correctly inside forwardRef and context consumer #12777

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
6 participants
@sophiebits
Copy link
Contributor

sophiebits commented May 10, 2018

Previously, _owner would be null if you create an element inside forwardRef or inside a context consumer. This is used by ReactNativeFiberInspector when traversing the hierarchy and also to give more info in some warning texts. This also means you'll now correctly get a warning if you call setState inside one of these.

Test Plan: Tim tried it in the RN inspector.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented May 10, 2018

Does the debug thing need to be wrapped in DEV?

@sophiebits

This comment has been minimized.

Copy link
Contributor

sophiebits commented May 10, 2018

Yeah, you're right.

@sophiebits sophiebits force-pushed the sophiebits:debugowner branch 2 times, most recently from b2cd16a to 2fed93d May 10, 2018

@pull-bot

This comment has been minimized.

Copy link

pull-bot commented May 10, 2018

Details of bundled changes.

Comparing: a9abd27...e8394da

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% 0.0% 614.9 KB 615.29 KB 142.2 KB 142.21 KB UMD_DEV
react-dom.development.js +0.1% 0.0% 599.28 KB 599.67 KB 138.05 KB 138.06 KB NODE_DEV
ReactDOM-dev.js +0.1% 0.0% 623.75 KB 624.14 KB 140.84 KB 140.86 KB FB_WWW_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% 0.0% 417.84 KB 418.23 KB 91.16 KB 91.17 KB UMD_DEV
react-art.development.js +0.1% 0.0% 343.67 KB 344.07 KB 72.49 KB 72.5 KB NODE_DEV
ReactART-dev.js +0.1% 0.0% 351.97 KB 352.36 KB 72.08 KB 72.11 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% 0.0% 348.71 KB 349.1 KB 73.38 KB 73.39 KB UMD_DEV
react-test-renderer.development.js +0.1% 0.0% 339.54 KB 339.93 KB 70.67 KB 70.68 KB NODE_DEV
ReactTestRenderer-dev.js +0.1% 0.0% 348.07 KB 348.46 KB 70.31 KB 70.34 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% 0.0% 319.89 KB 320.28 KB 66.12 KB 66.13 KB NODE_DEV
react-reconciler-persistent.development.js +0.1% 0.0% 319.22 KB 319.61 KB 65.88 KB 65.89 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% 0.0% 459.69 KB 460.08 KB 98.35 KB 98.37 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.1% 0.0% 459.44 KB 459.83 KB 98.29 KB 98.31 KB RN_OSS_DEV
ReactFabric-dev.js +0.1% 0.0% 442.1 KB 442.49 KB 93.95 KB 93.96 KB RN_FB_DEV
ReactFabric-dev.js +0.1% 0.0% 442.13 KB 442.52 KB 93.96 KB 93.98 KB RN_OSS_DEV

Generated by 🚫 dangerJS

Set owner correctly inside forwardRef and context consumer
Previously, _owner would be null if you create an element inside forwardRef or inside a context consumer. This is used by ReactNativeFiberInspector when traversing the hierarchy and also to give more info in some warning texts. This also means you'll now correctly get a warning if you call setState inside one of these.

Test Plan: Tim tried it in the RN inspector.

@sophiebits sophiebits force-pushed the sophiebits:debugowner branch from 2fed93d to e8394da May 10, 2018

@yungsters
Copy link
Contributor

yungsters left a comment

Awesome. Thanks for fixing this so quickly.

@sophiebits sophiebits merged commit 2a4d2ca into facebook:master May 14, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

let nextChildren;
if (__DEV__) {
ReactCurrentOwner.current = workInProgress;

This comment has been minimized.

@sebmarkbage

sebmarkbage May 14, 2018

Member

This doesn't look right. We shouldn't set current owner, which is runtime behavior, only in DEV. This could mess up certain new features in prod for example.

This comment has been minimized.

@sebmarkbage

sebmarkbage May 14, 2018

Member

We were supposed to use the debug frame thing for DEV warnings instead of owner so that we didn't have to do this. Perhaps the fix is to change how the context warning works so that it doesn't rely on current owner? @acdlite

This comment has been minimized.

@sophiebits

sophiebits May 14, 2018

Contributor

oh hm. for functional and indeterminate components it looks DEV-only:

ReactCurrentOwner.current = workInProgress;
ReactDebugCurrentFiber.setCurrentPhase('render');
nextChildren = fn(nextProps, context);
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
nextChildren = fn(nextProps, context);
}
// React DevTools reads this flag.
workInProgress.effectTag |= PerformedWork;
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
}
function updateClassComponent(
current: Fiber | null,
workInProgress: Fiber,
renderExpirationTime: ExpirationTime,
) {
// Push context providers early to prevent context stack mismatches.
// During mounting we don't know the child context yet as the instance doesn't exist.
// We will invalidate the child context in finishClassComponent() right after rendering.
const hasContext = pushLegacyContextProvider(workInProgress);
let shouldUpdate;
if (current === null) {
if (workInProgress.stateNode === null) {
// In the initial pass we might need to construct the instance.
constructClassInstance(
workInProgress,
workInProgress.pendingProps,
renderExpirationTime,
);
mountClassInstance(workInProgress, renderExpirationTime);
shouldUpdate = true;
} else {
// In a resume, we'll already have an instance we can reuse.
shouldUpdate = resumeMountClassInstance(
workInProgress,
renderExpirationTime,
);
}
} else {
shouldUpdate = updateClassInstance(
current,
workInProgress,
renderExpirationTime,
);
}
return finishClassComponent(
current,
workInProgress,
shouldUpdate,
hasContext,
renderExpirationTime,
);
}
function finishClassComponent(
current: Fiber | null,
workInProgress: Fiber,
shouldUpdate: boolean,
hasContext: boolean,
renderExpirationTime: ExpirationTime,
) {
// Refs should update even if shouldComponentUpdate returns false
markRef(current, workInProgress);
const didCaptureError =
(workInProgress.effectTag & DidCapture) !== NoEffect;
if (!shouldUpdate && !didCaptureError) {
// Context providers should defer to sCU for rendering
if (hasContext) {
invalidateContextProvider(workInProgress, false);
}
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
const ctor = workInProgress.type;
const instance = workInProgress.stateNode;
// Rerender
ReactCurrentOwner.current = workInProgress;

I guess that's an oversight?

This comment has been minimized.

@gaearon

gaearon May 14, 2018

Member

I think at the time this was intentional because we didn't separate those concepts, and didn't want to have the overhead of setting a property in PROD when it doesn't end up being used (functional components can't be ref owners which was the only runtime behavior of this feature).

This comment has been minimized.

@sebmarkbage

sebmarkbage May 14, 2018

Member

We just have to be better at following through when we change strategies like introducing debug frame.

bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request May 24, 2018

Merge #20
20: Update react monorepo to v16.4.0 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

bors bot added a commit to mozilla/delivery-console that referenced this pull request May 24, 2018

Merge #164
164: Update react monorepo to v16.4.0 r=rehandalal a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

TejasQ added a commit to TejasQ/react that referenced this pull request Aug 26, 2018

Set owner correctly inside forwardRef and context consumer (facebook#…
…12777)

Previously, _owner would be null if you create an element inside forwardRef or inside a context consumer. This is used by ReactNativeFiberInspector when traversing the hierarchy and also to give more info in some warning texts. This also means you'll now correctly get a warning if you call setState inside one of these.

Test Plan: Tim tried it in the RN inspector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment