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

Allow internal components to be used in the browser version #2251

Closed
totty90 opened this issue Sep 26, 2014 · 10 comments
Closed

Allow internal components to be used in the browser version #2251

totty90 opened this issue Sep 26, 2014 · 10 comments

Comments

@totty90
Copy link

totty90 commented Sep 26, 2014

I'm using flux and I need the 'react/lib/copyProperties'. This is easy with browserify var copyProperties = require('react/lib/copyProperties'); but with require.js is not possible because it uses the bower version which doesn't contain the copyProperties fn (https://github.com/facebook/react/blob/aae31ae24c67216894ae42b8481a599206d18690/src/browser/ui/React.js). Would be nice to have them like this React.lib.copyProperties()

Maybe include all the lib components.

Is this possible?
Thanks

@syranide
Copy link
Contributor

This would affect minification and it's an internal API that's subject to change at any time. If you want a function, use your own copy I'd say. (PS. Also use webpack instead of require.js :))

@totty90
Copy link
Author

totty90 commented Sep 26, 2014

But in browserify with npm you can use them, isn't that coherent? Just because is bower I can't use it?
Maybe an internal API would be enough, even if changes. I know the functions are in the minified version, so you just have to export them somehow.

@syranide
Copy link
Contributor

@totty90 "can" and "should" are two different things. They're not publicly exposed, by reaching into react/* you are accessing internal modules, React offers no guarantees.

@totty90
Copy link
Author

totty90 commented Sep 27, 2014

@syranide Look at your flux example:

https://github.com/facebook/flux/blob/master/examples/flux-chat/js/stores/MessageStore.js

var merge = require('react/lib/merge');

https://github.com/facebook/flux/blob/master/examples/flux-chat/js/constants/ChatConstants.js

var keyMirror = require('react/lib/keyMirror');

So, you are only giving the benefits to the npm version, while hiding from the bower version. Would be better if both would have the same structure. In this way if someone goes from npm to bowser version doesn't have to change all the stuff.

  • You provide a tutorial that uses internal stuff, but then you say is internal and should not be used?
  • I think internals should also have "_" prepended at least to be understood that are internals, because those seems public.

Is your decision whenever to add it or not, but if you decide to don't add it, then please notice the flux guys to make a tutorial with the API modules instead of the internal ones. You say "do this like that, but you shouldn't" or in our context "use require('react/lib/keyMirror'), but don't use internal API".

@syranide
Copy link
Contributor

I'm not an official dev so that's not my decision. I imagine that it was simply with the intent of keeping the meat of the examples simple rather than having link to all the different implementations.

It's not React's purpose and it's rather arbitrary; what should be exposed and what shouldn't. Should helpers not useful to React but users also be exposed? Should library X also expose merge, which one should you use? What if React no longer needs merge? What if React suddenly requires a specific semantic implementation of merge?

I think the idea that React should expose helper methods is flawed, if anything, it should probably be the other way around. React should depend on shared modules, if you depend on the same module and your dependency requirements are compatible you would share the implementation.

Again I'm not on "Team React" so this is not my decision.

@totty90
Copy link
Author

totty90 commented Sep 27, 2014

Yes, this seems a better approach. I also thought about the problem when react does't need a lib anymore.

@syranide
Copy link
Contributor

syranide commented Oct 8, 2014

@totty90 This is not my responsibility, but if you want to submit an issue to petition for moving share-able utils from core into separate packages then that seems sound to me (not that I would expect it to happen anytime soon).

@syranide
Copy link
Contributor

Apparently it's "kind of" happening #2317

@zpao
Copy link
Member

zpao commented Oct 10, 2014

Not going to do this. trying to move away from this as much as possible. Flux examples should probably not use the internal modules.

@zpao zpao closed this as completed Oct 10, 2014
@zpao
Copy link
Member

zpao commented Oct 10, 2014

Filed facebookarchive/flux#77 for Flux.

doctyper pushed a commit to nfl/react-helmet that referenced this issue Jun 28, 2015
React’s official policy on these helpers is “use at your own risk”:
- facebook/react#1906 (comment)
- facebook/react#2251 (comment)
- facebook/react#2317
power1220 added a commit to power1220/react-helmet that referenced this issue Apr 4, 2023
React’s official policy on these helpers is “use at your own risk”:
- facebook/react#1906 (comment)
- facebook/react#2251 (comment)
- facebook/react#2317
LucSPI added a commit to LucSPI/React-Helmet that referenced this issue Feb 12, 2024
React’s official policy on these helpers is “use at your own risk”:
- facebook/react#1906 (comment)
- facebook/react#2251 (comment)
- facebook/react#2317
AMagicHarry added a commit to AMagicHarry/react-helmet that referenced this issue Mar 11, 2024
React’s official policy on these helpers is “use at your own risk”:
- facebook/react#1906 (comment)
- facebook/react#2251 (comment)
- facebook/react#2317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants