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

Dependency inject React. #2031

Closed
wants to merge 1 commit into from
Closed

Conversation

skevy
Copy link

@skevy skevy commented Sep 21, 2015

Allows use with React Native or other custom React version. I followed the same pattern as rackt/react-redux.

The only breaking change with this (since all tests pass, and you guys write awesome tests) would be for people who import directly from "react-router/lib/[something]" and use private APIs. I'm not sure what your thoughts would be on what to do about those people.

I'm also aware that you might not even want to accept a PR like this, given the fact that one day (though none of us know what day that is haha) we can all depend on React >= 0.14, and react-router can depend on just react (sans react-dom), and that will (hopefully) just work with react-native with no changes needed. Ahhh, the future. It will be so nice.

But for using react-router on RN today, this is needed.

Allows use with React Native or other custom React version.
@timdorr
Copy link
Member

timdorr commented Sep 21, 2015

Ignoring whitespace makes the commit more readable.

Could you not just hook into your build tool and manually resolve the "react" library? We do this in the redux example code to use the local version instead of the one from the package repo.

@skevy
Copy link
Author

skevy commented Sep 21, 2015

@timdorr unfortunately, you can't do that with React Native, as far as I'm aware. It doesn't use webpack (or anything like it) - it has it's own packager that does special dependency resolution.

@timdorr
Copy link
Member

timdorr commented Sep 21, 2015

Ah, gotcha. I'm not familiar with react-native's packaging system.

I would say use react 0.14.0-rc1 now, since it shouldn't break APIs when it goes final. Heck, Facebook is using it in production even! 😄

screen shot 2015-09-21 at 2 16 05 pm

@skevy
Copy link
Author

skevy commented Sep 21, 2015

Unfortunately, that's not how RN works. You have to use the bundled version of react that's built in. I presume eventually they will upgrade to 0.14.0 and split off the native component, but it's my understanding that it's going to be a bit until that happens.

@timdorr
Copy link
Member

timdorr commented Sep 21, 2015

Welp, I'm having no luck in this conversation 😄

I would say this significantly impacts a number of use cases that I and others have with the package introspection that you mentioned. Perhaps a better approach would be to simply change all the react imports to a local module that can be configured in some way to pass back any object as React.

@timdorr
Copy link
Member

timdorr commented Sep 22, 2015

I found this thing: https://github.com/mjohnston/react-native-webpack-server Does that help the situation?

@taion
Copy link
Contributor

taion commented Sep 23, 2015

@ryanflorence @mjackson This was the PR I mentioned for React Router + RN.

@skevy
Copy link
Author

skevy commented Sep 23, 2015

@timdorr definitely familiar with that project and have used it a few times. Problem is that 1) it's unsupported and 2) it's buggy. In general, not really a super viable option (at least in my opinion) for production ready RN apps. At least at the moment. Hopefully that will change. :-)

So the options I see are the following:

  1. Modify this PR sufficiently to address and support people who currently use private APIs.
  2. Work with the RN packager team to allow for a way to alias packages in their packager, such as is described here: support react-native field with fallback to browser field facebook/react-native#2208
  3. Work with the RN team to get RN to use react 0.14.

I would rank these in order of difficulty from easiest to hardest, respectively. As far as the most "right" path forward? I think it's hands down # 3.

To allow people to use RR on RN today? Would probably be the first option. It's really up to the maintainers on the direction they want to go with the lib.

Really just was aiming to share this PR to start discussion on react-router on RN.

Oh btw, it basically "just works", if you ignore for a moment integrating with Navigator and TabBarIOS. Those integrations are more difficult (I have working versions of each, but need to work to test them and continue to make them stable). Point is, it's possible and super doable, providing there's some workaround for this packaging problem.

@mjackson
Copy link
Member

Thanks for putting so much effort and thought into this, @skevy! TBH, I was just kind of ignoring the whole packaging problem, hoping that the RN team would come around and support 0.14 and we could all sort of just make the leap at the same time. But that may not be practical.

I wonder if @vjeux knows what React Native's plans re: 0.14 are?

@gaearon
Copy link
Contributor

gaearon commented Sep 24, 2015

I read in a tweet that after 0.14, a compatible RN version may arrive in a few weeks.

@gaearon
Copy link
Contributor

gaearon commented Sep 24, 2015

definitely familiar with that project and have used it a few times. Problem is that 1) it's unsupported and 2) it's buggy.

Some race conditions have been fixed lately so maybe you should give it another try.

@skevy
Copy link
Author

skevy commented Sep 24, 2015

@mjackson @gaearon fwiw @ide made an issue about the 0.14 migration last night, for reference: facebook/react-native#2985

TLDR; it's priority for them, but no timeline yet (it seems).

@timdorr
Copy link
Member

timdorr commented Oct 12, 2015

@skevy Since react-router is now using 0.14, does this look to OK to close now?

@skevy
Copy link
Author

skevy commented Oct 12, 2015

I think it's fine to close. There's a bigger discussion to be had about actually integrating with the React Native and the direction that that could take...but @mjackson and I are supposed to offline about that at some point in the very near future. I also need to keep harping on facebook/react-native#2985 to make sure that actually happens, in order to make stuff like this possible.

@timdorr
Copy link
Member

timdorr commented Oct 12, 2015

Yeah, that sounds like some great RN ecosystem enhancement. Hope they can get to it soon!

@timdorr timdorr closed this Oct 12, 2015
@taion taion mentioned this pull request Nov 7, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants