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

Rename react/lib/* to react/private/* #6460

Closed
gaearon opened this issue Apr 8, 2016 · 19 comments
Closed

Rename react/lib/* to react/private/* #6460

gaearon opened this issue Apr 8, 2016 · 19 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

As suggested on Twitter.
Let’s do it so we don’t get more broken components?

@zpao
Copy link
Member

zpao commented Apr 8, 2016

I still like the lib/ pattern 😢 It might be slightly more obvious to people not to require files from private/ but nothing really stopping them and we'd end up telling people the same thing - "don't require things from here".

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 8, 2016

While we have been telling this to people, the reality is that a lot of people (me included) thought it was relatively safe some time during 0.12, and there is a bunch of components whose authors don't know this needs changing.

Renaming it for 16 would serve as a good warning for any existing components, as it would effectively force authors to explicitly opt into using "private" parts or find workarounds.

@zpao
Copy link
Member

zpao commented Apr 8, 2016

We could go all in and really obfuscate it. Some hash or something that gets added to our build pipeline and changes for every major release.

echo '16' | md5
5b6b41ed9b343fed9cd05a66d36650f0

@jimfb
Copy link
Contributor

jimfb commented Apr 8, 2016

👍 :%s/major release/build/g

@zpao
Copy link
Member

zpao commented Apr 8, 2016

Nah, shouldn't do it on every build. I thought about it but that would probably screw our own packages so we need some reliability (eg react-dom 15.0.0 should be able to use react 15.0.1).

@jimfb
Copy link
Contributor

jimfb commented Apr 8, 2016

Our own packages should have a way of accessing the internals needed, using private API like SECRET_DOM, without indexing into lib. That has the added bonus of allowing addons to be put into separate UMD builds.

@iamdustan
Copy link
Contributor

while there isn’t a defined, documented API for accessing internals to create a renderer, this change will greatly hinder all the experimentation taking place in the custom react renderers community. While that is a known experimental use case where things will break (I believe maintainers and users of these renderers understand this), removing that capability or hiding it in a system like md5 hash names would be detrimental in my view.

@jimfb
Copy link
Contributor

jimfb commented Apr 8, 2016

I feel like people experimenting should check out the React repository anyway, rather than relying on an NPM build. Maybe I'm wrong, curious what @gaearon thinks about that point.

@zpao
Copy link
Member

zpao commented Apr 8, 2016

Depending on how we did it, I don't think it would really hinder experimentation. You would just be forced into knowingly using non-public APIs and if we went down that hash per major route, you'd just have to update paths for a major version. We'll definitely keep the use case in mind though. And you're paying attention so I'm sure you'll chime in (or I'll reach out) if we make any drastic changes that would be unrecoverable.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 8, 2016

I feel like people experimenting should check out the React repository anyway, rather than relying on an NPM build.

Building it and then doing something like npm link can be a pain considering you’d want the renderer to be a peer dep, and to both run tests in it and experiment on some project in another directory. This is when symlinks get too inconvenient. I definitely see the appeal of just using the npm package when fiddling with the renderers.

Depending on how we did it, I don't think it would really hinder experimentation. You would just be forced into knowingly using non-public APIs and if we went down that hash per major route, you'd just have to update paths for a major version.

Something like react/lib/5b6b4/* (5 chars would be enough IMO) wouldn’t be too bad if we communicated clearly what this is. We could ship a react/lib/README.md that explains that the hash changes on every major version.

@iamdustan
Copy link
Contributor

I’ve thought about this a bit more over the weekend and am currently opposed to this. The reasoning against this is:

  1. It feels like a kneejerk reaction to a community problem. While it is a valid solution (with a precedent by babel doing something similar), I suspect that community pressure and education will be enough to prevent or decrease the impact of this behavior happening as time progresses.
  2. It breaks conventions and makes a certain class of valid use cases more difficult. As mentioned, it even requires thought and additional work for your own OSS products with RN
  3. It’s a bit of a hack to implement truly private modules. And even with that it isn’t fully private because maintainers could look at the docs for how the hash is generated and still require private internals. Ideally, if we want to add a truly private concept to the module system, it should be built in to npm and/or node require itself.

@blainekasten
Copy link
Contributor

An API that could satisfy everyones needs:

const reactLibHash = require('react/libHash'); // e.g., "M0D3F"
const libIWanted = require(`react/lib/${reactLibHash}/ThingIWant`);

This would let you use the hashing method, while still allowing people to get the things they want from libs. But making it awkward enough that it should hopefully be only used when actively sought after. Not documenting it could help with that.

Downsides are that it doesn't work with the import system as you can't interpolate variables into the import path.

Ultimately, I agree with @iamdustan on every point. It seems like a user problem more than your teams problem. If somebody wants to depend on an internal item and it gets removed in the next version, then it's up to them to handle that situation over you guys. That's an OSS-wide issue, not just a react issue.

@toxicFork
Copy link

Agree completely with @iamdustan as a custom renderer author. Currently it is necessary for my project to use react internals and I am aware that it may break even with patches or minor updates to react but that's something I can handle for now. Having build specific hashes would just make it a bad experience for almost everyone.

Naming it "private" instead of "lib" will make it more apparent that things may break so that's good enough in my opinion.

To solve the real problem at least in my case is to find out why the third parties are trying to use private parts of react.

To be even more specific, I am seeing great progress towards separation of DOM and react, and for react three renderer, with every update, fewer private react parts need to be used.

Eventually when react dom is completely separated we will be able to look at it and see how it works as an independent entity and if it does not touch react private parts then it will be very comfortable for other custom renderers to appear and be more stable.

I am guessing that eventually the same may happen to the other popular private parts, I guess they may need to be exposed in the stable api or modularized. Or third parties could be poked. Poke us but don't punish us :)

@blainekasten
Copy link
Contributor

Another factor to consider. There are recordings of you guys considering Enzyme to be officially supported as a community based library for testing React. There are parts of Enzyme that currently require reaching into react. So either we would need to work together on some of that, or need continued access into the internals.

Example:
https://github.com/koba04/enzyme/blob/simulate-supports-batched-updates/src/react-compat.js#L137
https://github.com/koba04/enzyme/blob/master/src/react-compat.js#L24

@gaearon
Copy link
Collaborator Author

gaearon commented May 10, 2016

Another factor to consider. There are recordings of you guys considering Enzyme to be officially supported as a community based library for testing React. There are parts of Enzyme that currently require reaching into react. So either we would need to work together on some of that, or need continued access into the internals.

Yeah, absolutely. From the related core notes:

Challenges

  • Ideally we want test runner to be a separate renderer.
  • Tests should run consistently and often synchronously so we don’t want to run them with incremental reconciler.
  • We have no official renderer APIs yet so without TestUtils Enzyme would have to rely on React internals.

We understand this is the case now, too. We’ll be keeping this in mind as we figure out the strategy for how to move further with #4230 without breaking everyone.

@syranide
Copy link
Contributor

Something like react/lib/5b6b4/* (5 chars would be enough IMO) wouldn’t be too bad if we communicated clearly what this is. We could ship a react/lib/README.md that explains that the hash changes on every major version.

@gaearon If that's the case, doesn't it make more sense to just call it react/lib/v15/, react/lib/v16/? But both of those still kind of invite people to use them, neither screams off-limits, they would mostly just think "why did they do that?" and continue. Personally I would say just react/private/ is enough though, for whatever it's worth.

@aweary
Copy link
Contributor

aweary commented May 10, 2016

@syranide @gaearon it would be nice if the directory name corresponded to the ReactVersion value if that's the case.

@zpao
Copy link
Member

zpao commented May 10, 2016

It's worth noting that this is only relevant as long as we use @providesModule, which might not be for much longer (#6336). At that point we'll probably be using relative requires and perhaps go even further with a flat bundling approach where our npm module is a single file. There are things to work out and people/projects to work with before we finalize anything regardless. So I think it's probably not worth changing things multiple times and thus I doubt we'll do anything about this.

@gaearon
Copy link
Collaborator Author

gaearon commented May 10, 2016

👍 Let’s close it then in favor of #6336 and #6351.

@gaearon gaearon closed this as completed May 10, 2016
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

8 participants