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

Move getNextDebugID() to React package. #9005

Closed
wants to merge 1 commit into from

Conversation

wacii
Copy link
Contributor

@wacii wacii commented Feb 15, 2017

See #8487

When multiple renderers are used in development, react issues a warning from ReactComponentTreeHook.

Why?

Items are stored in ReactComponentTreeHook's itemMap for debug info during development. The keys for these entries are generated by the following code.

var nextDebugID = 1;

function getNextDebugID() {
  return nextDebugID++;
}

The problem is both ReactDOM and ReactDOMServer have a copy of this, and so will generate the same series of numbers, overwriting the debug entries generated by the other.

Now it lives only in React.

@gaearon
Copy link
Collaborator

gaearon commented Mar 1, 2017

Thanks! I added this to list of changes we want to push out in 15.5.
Do you think you could rebase this on master?

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Don't like. Another stateful module in isomorphic wish we tried hard to get away from (e.g. current owner). :(

@gaearon
Copy link
Collaborator

gaearon commented Mar 1, 2017

It should die with the tree hook, I think?

@wacii wacii force-pushed the move-next-debug-id branch 2 times, most recently from 5cb42d9 to e6f79f1 Compare March 2, 2017 20:07
The module, and its internal state, was duplicated in several packages,
leading to duplicate ids when multiple renderers were used.
@wacii
Copy link
Contributor Author

wacii commented Mar 3, 2017

The code is now up to date.

@gaearon gaearon added this to the 15-hipri milestone Mar 3, 2017
@@ -0,0 +1,9 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this file? Looks like a build product that shouldn't be checked in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #9078.

It does look that way. But shared modules are now accessed under their package name, so require('react/lib/getNextDebugID') instead of require('getNextDebugID'). Why this was done with an extra module, what @sebmarkbage referred to as forwarding modules I believe, and not an alias...I have no idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea. This is correct. When we switch to lerna or yarn workspaces, this will be generated by tooling but since we don't have the tooling in place yet, I just checked in forwarding modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Forwarding modules is so much better than config aliases because it covers all tooling at once. E.g. webpack aliases are the worst because you have to replicate it in node, jest, flow, and anything else that need the module graph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aah. Thanks for explaining.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(That's the same reason we don't allow absolute imports via Webpack config in CRA 😄 )

@acdlite
Copy link
Collaborator

acdlite commented Apr 6, 2017

I cherry-picked this onto our release branch. It will go out in 15.5. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants