-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Split React and ReactDOM from npm build. #6022
Conversation
current: null, | ||
|
||
}; | ||
var ReactCurrentOwner = require('react-current-owner'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work out of the box in our primary @providesModule
world. We could make it but it's trickier and more prone to breakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I test that? Or is the fact that grunt build:basix
fails indicative of it being incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't, unless you want to come work at FB :) Introducing a normal node require into @providesModule
code requires more work and doing things in a different part of the codebase that just means we have a little bit more to keep in mind when we sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remotefriendlycoughcough
Would abusing the RN packager be analogous to haste tooling? I dug around a bit today and noticed this current impl is a bit lacking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a matter of having to do special configs for these things in our website code to work. Nothing else to do about it.
In this code base you should be able to make it work - we have to hard code that react-current-owner
is normal and should not get rewritten. And you have to do that in the gulpfile (again, sorry for the crazy build system). https://github.com/facebook/react/blob/master/gulpfile.js#L42 - we should make that Object.assign({}, require('fbjs/module-map'), {'react-current-owner': 'react-current-owner'})
. Basically the build assumes that if something isn't on the whitelist, then it must be a providesModule module. We dump all our modules into lib/ and rewrite the requires so they are relative. So this is currently getting rewritten to require('./react-current-owner')
which is going to fail when browserifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn’t there some work to reign in the build system somewhere?
Updated with that and now grunt build
works correctly, the browser examples each work, and I can npm link
this into my custom renderer and require react
directly instead of react/lib/ReactIsomorphic
.
This seems like a pretty reasonable first step, will just need to make sure we do the |
@iamdustan updated the pull request. |
is there a timeline or active work regarding this? Tbh, my motivation for this separation is for React Hardware which is already dancing with internals enough as it is :) |
7bedfd9
to
ff7d9f1
Compare
@iamdustan updated the pull request. |
Testing this locally, I bumped to Is there anything I can do regarding haste? This PR was able to remove one of two *.native.js shim files added in c29642d According to reduxjs/react-redux#292 (comment) it sounds like the Relay dependency on the second file is handled, so only the |
I'm inclined to say that we aren't going to get to this this week and it'll miss v15 :( I think there are just a few too many unknowns with npm and how the 3rd package should be depended on and shared (eg, I would expect with npm2 that react and react-dom each install their own react-current-owner, which would not be what we want). I think requiring that react-current-owner becomes installed by apps would really awkward. |
Based on my understanding, it would be more expedient and less invasive to the React codebase to deprecate and eventually remove the CurrentOwner concept than attempt to continue finding a way to create another package for React consumers to depend on. Especially given npm@2 and npm@3’s differing trees I could see this become a source of confusion in the future. If that is an accurate assessment of the direction React is heading I would vote for closing this and the original issue (#5974) in favor of the CurrentOwner cleanup. #6331 already solves the npm side of #5974 won, and if CurrentOwner didn’t exist I don’t think there is anything else that would be impeding #5974 from being complete. |
Agreed. I’m closing for this reason. Thanks for checking the viability of this! |
See #5974.
This solves the first two parts of #5974.
The “common” consumption method from npm should have a fully separated renderer with this. @providesModule I think would also continue operating as expected. The only deferred work is separating the browser builds.
I’m not sure what the demand for the stand-alone browserified builds is, but that is definitely much more work with the build tooling. Personally, I’d be happy to see this be the next step in the separation of the two. It’s lower risk as it only modifies two of the three worlds (npm and haste). It trades the React.native.js file for a ReactBrowser.js file which sounds like a decent tradeoff to me.
This isn’t quite there yet...I have to work through the grunt tasks to ensure the browser builds still work...