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

Expose ReactMount? #3568

Closed
ghost opened this issue Apr 2, 2015 · 13 comments
Closed

Expose ReactMount? #3568

ghost opened this issue Apr 2, 2015 · 13 comments

Comments

@ghost
Copy link

ghost commented Apr 2, 2015

Sorry if this already exists in some form or if I'm misunderstanding how everything works, but certain libraries (e.g., react-hot-api), depend on the use of require('react/lib/ReactMount')._instancesByReactRootID, which won't work if the only script available is the React bundle (e.g., react-with-addons-0.13.1.js).

For my own projects, I've added a single line to essentially src/browser/ui/React.js to expose ReactMount as React.Mount, so that require('react').Mount._instanceByReactRootID will work. It would be nice to have this available by default, however. Apologies if it's available by default in some other form.

@ghost ghost mentioned this issue Apr 2, 2015
@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

BTW it's available if you register as a dev hook, but IIRC there can only be one dev hook. I really want React to have better tooling/introspection story.

@ghost
Copy link
Author

ghost commented Apr 2, 2015

I saw that it's exposed within __REACT_DEVTOOLS_GLOBAL_HOOK__ which is used by the react-devtools Chrome extension, right? Is this what you're referring to?

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

Yes. If such hook is defined, React will give its internals to it. The problem is, DevTools already defines it, and you don't want your library/tool to conflict with DevTools.

@jimfb
Copy link
Contributor

jimfb commented Apr 2, 2015

Correct, React dev tools reaches into the internal private API. We don't expose that hook as public API because it is subject to change without notice and we don't want people relying on it (to the extent possible). That said, @timbur, exposing ReactMount the way you did is a pretty reasonable solution if you really need access to the internals; just be aware that it is "unsupported" territory. As per @gaearon's comment, yes, as we grow it might make sense to have better resources for dev tooling. It would be interesting to start understanding the tooling needs/usecases to see what could be better addressed by our devtools, and what/why people need whatever isn't yet available. @timbur I'm a little curious about the details of your use case.

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

cc @syranide who I remember talked about it before

@ghost
Copy link
Author

ghost commented Apr 2, 2015

@JSFB I'm putting together an ES6+JSX+React development toolkit that includes an optional dev tools extension where you can use Webkit's/Firefox's native sources panel+editor to update scripts (modules) with hot reloading. There's also real-time replication so devs have the option to work on the same app at the same time. And if for some reason someone can't/won't install the extension, the option to update modules from within the browser itself via CodeMirror is available, so it makes sense to include the hot reload logic within the usual window rather than the dev tools. The extension passes the changed code onto the inspected window which reloads the module. And to me, React.Mount seems like a cleaner solution than relying on __REACT_DEVTOOLS_GLOBAL_HOOK__ some way or another.

@zpao
Copy link
Member

zpao commented Apr 2, 2015

We really want to keep the public API minimal, so I don't think we're going to expose ReactMount directly. We may do it in a way that is very obviously unsupported (we did that before with React.__internals, also with devtools in mind), but the way you have it is not how we'll do it.

If all you actually need is _instanceByReactRootID (which note, is even "private" within that module), then we should figure that out.

@ghost
Copy link
Author

ghost commented Apr 2, 2015

I honestly think the way _instancesByReactRootID is being used for the hot reloading is pretty hacky, but I'm guessing it's necessary due to there being no methods designed specifically for deeply traversing through React elements matching some class? A few simple methods for this would probably be ideal for dev tools.

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

I'm guessing it's necessary due to there being no methods designed specifically for deeply traversing through React elements matching some class?

The class doesn't even matter, all I need is the access to all the root real React component instances on the page. (Please don't merge #2958 until this is solved in some way.)

@ghost
Copy link
Author

ghost commented Apr 2, 2015

So would it work to maybe cache all real root component instances and expose that in some way? And the real question might be, would this be all that other potential dev tools need as well (for whatever arbitrary things that devs might want to change/bind)?

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

I'd love to see a first-class officially supported React introspection API that would be usable by both Devtools and other libraries. There's not much to support: give us internal instance tree, a way to force updates and maybe a few more things. What keeps this from happening, aside from somebody writing it?

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2015

To clarify: I understand “minimal API surface area” but I can't see how making this introspection API slightly more official would hurt. Nobody expects them to be stable. Even with them changing with every release, it would still be an improvement over ad-hoc solutions we use now.

@ghost
Copy link
Author

ghost commented Apr 2, 2015

Indeed! Necessity (or lack thereof) is probably the only thing delaying it. Things seem to be working well enough as is, despite some slightly hacky stuff. That said, I'd be glad to take a stab at it myself, but I'm not sure when I'll have the time. Plus, I wouldn't want to waste my time so perhaps someone could outline a few specs that would for sure be merged if implemented?

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

Successfully merging a pull request may close this issue.

3 participants