Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Conversation

@gaearon
Copy link
Contributor

@gaearon gaearon commented Aug 20, 2018

This updates DevTools to work with constants that were changed in facebook/react#13397. Note you'll need to change ReactVersion to 16.4.3 in a locally compiled ReactDOM to test the new code path.

I decided to pull all constants and Fiber-specific helpers in one closure that has access to the version. We might tweak more things over time, and I think it'll be easier to update if everything is in one place.

You might find reviewing individual commits easier.

This will make it easier to change them in the future based on the version.
function enqueueMount(fiber) {
pendingEvents.push({
internalInstance: getOpaqueNode(fiber),
data: getDataFiber(fiber, getOpaqueNode),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now in the closure anyway.

@bvaughn
Copy link
Contributor

bvaughn commented Aug 20, 2018

Got some lingering references to ReactTypeOfWork.js and ReactSymbols.js in backend/getDataFiber.js that are failing CI

@bvaughn
Copy link
Contributor

bvaughn commented Aug 20, 2018

In general, I think the approach to forking based on version number is fine 👍

Both attachRendererFiber and getDataFiber need access to these constant values though, so we'll have to export them somehow.

@gaearon
Copy link
Contributor Author

gaearon commented Aug 20, 2018

Oops, I meant to delete that file.

@gaearon
Copy link
Contributor Author

gaearon commented Aug 20, 2018

Both attachRendererFiber and getDataFiber need access to these constant values though, so we'll have to export them somehow.

I inlined getDataFiber into attachRendererFiber (right into the closure).

If there's still a getDataFiber.js file this sounds like a mistake. Will check now.

@gaearon
Copy link
Contributor Author

gaearon commented Aug 20, 2018

Fixed

@bvaughn
Copy link
Contributor

bvaughn commented Aug 20, 2018

Oh, great! I didn't notice that it was gone entirely, although I did see your comment about the closure and...apparently misunderstood it. Cool 👍

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ++/-- line count also makes more sense now 😁

@gaearon gaearon merged commit ca17845 into master Aug 20, 2018
@gaearon gaearon deleted the support-newer branch August 20, 2018 15:19
OrDuan pushed a commit to OrDuan/react-devtools that referenced this pull request Aug 23, 2018
* Inline all Fiber-specific parts in attachRendererFiber

This will make it easier to change them in the future based on the version.

* Switch React constants by the version

* Delete accidentally left file
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants