AUTO IP Implementation #4681
AUTO IP Implementation #4681
Conversation
|
By analyzing the blame information on this pull request, we identified @foghina, @frantic and @nicklockwood to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
Note: init() method was removed in master version of RCTWebSocketExecutor.m. The IP detection logic was implemented in setup() instead. For more details on the functionality: http://moduscreate.com/automated-ip-configuration-for-react-native-development/ I added #undef (which is commented out) so people this doesn't work for can use the existing 0.16 logic. |
| #ifdef AUTO_IP | ||
| NSString *serverIP = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"SERVER_IP"]; | ||
| NSInteger port = [standardDefaults integerForKey:@"websocket-executor-port"] ?: 8081; | ||
| NSString *debugUrlString = [NSString stringWithFormat:@"http://%@:%zd/debugger-proxy", serverIP, port]; |
There was a problem hiding this comment.
This'll fail when running in the simulator... you need to check for sim or device...
There was a problem hiding this comment.
It runs for me. However, I am using the 8.x simulator. You may be right about it not working in the 9.x simulators.
I'll look at the fix for this.
|
@mschwartz updated the pull request. |
|
Build failures appear to be related to broken node modules and gyp build for nodejs modules. |
|
The high-order feedback I have is that we want to decouple the WebSocketExecutor from the development environment instead of increasing the coupling. Do you see In the solution I would like to see three things:
|
|
Thanks for putting this together @mschwartz! I definitely agree with @ide. We need a more generalized solution that doesn't involve so much code living in both It would be great to see something like: jsCodeLocation = RCTGetDevelopmentBundleURL();...in As far as dealing with Thoughts? |
|
My thoughts are to accept the pull request and then make additional changes The PR isn't going to break anything and it only makes things a lot Make sense? In general, I think it would be a great thing if we could access the plist You obviously want to access these things from the WebScoketExecutor module On Thu, Dec 10, 2015 at 1:28 PM, Adam Miskiewicz notifications@github.com
|
|
Hi @mschwartz - It's imperative that the core contributor team consider the cost of each PR in addition to the immediate benefit it offers. While you're right that this PR doesn't break anything, we have to consider the technical debt that it imposes. The reason PR's are carefully reviewed (and the reason there are 192 sitting in the queue) is that In a project as large as RN, accepting PR's that aren't ideal solutions end up making more work for both the core team, as well as increasing the potential of confusing users of the framework. One of the places that React and React Native shine are in their API design. And it's not just the public facing APIs -- if you look at the work that people are putting into React Native every single day, just as much care is being put into internal APIs as public APIs, in an effort to make React Native more easy to maintain and more easy to contribute to in the future. In looking at PR's that we, as community core members, review and hopefully eventually accept, we want to continue this trend. That being said, I don't want to sound like we're bike-shedding here. The approaches that @ide and I presented are just examples...if a better or more elegant solution was presented, I would absolutely be OK with that. As was mentioned in #4245, I agreed to take this PR because I think it's an incredibly useful addition to core, and alleviates a lot of confusion that I've seen from beginner users of React Native. Despite it's usefulness, I don't want to encourage submitting and accepting less than ideal solutions. Hopefully you understand where we're coming from on this issue. Let me know how you'd like to proceed on this PR. If you don't have the bandwidth at the moment to work on the suggestions @ide and I mentioned, perhaps one of the other people that commented on #4245 would be willing to take this on? Thanks, |
|
@skevy where's the 'Like' button on this thing? ;-) |
|
@skevy , Careful review of PRs isn't the reason there's 192. This one for example: #1355 was reviewed, discussed and left hanging - since August! A huge majority of them are stalled for a myriad of reasons. The PR model seems to be pretty broken. You have folks who spend hours trying to come up with solutions to not only enhance the framework, but also enhance the developer community - only to get shot down because it's not the best or ideal solution. I agree that only merging "the best" solution is probably the greatest thing to do, but it's not the only reason why there's nearly two hundred PRs that have stalled. A solution needs to be found as this is starting to hurt the community. There are so many community-facing issues that need to be resolved. :( |
|
@jaygarcia I'm not sure what you mean. There's almost no discussion in that PR except that someone identified what seems like a fairly significant flaw in the implementation. |
|
@jaygarcia you're correct. It's not the sole reason. I should have been more explicit that it is one of the several reasons that there are 192 PR's. I won't beat around the bush -- it sucks that there are so many open issues and PR's. We're working on it. FB recently added me and several other more community members as core team members, and @vjeux and others at FB have specifically invested a ton of time in bots like the "shipit" bot in order to allow for PR's to be safely reviewed and merged by community members. It's going to take time and patience by the community to see this number drop. 10's of issues are opened every single day. At least 1 PR is opened every day. Every single person working on this project has a job that takes up the majority of their time. So even if I, personally, were to answer (and close) 10 issues a day, and accept or deny 1 PR a day...there would still be absolutely zero progress on the number of open issues and pull requests. Luckily, it's not just me, and we've been having discussions about how best to handle all of this work. I will reiterate, however, that accepting PR's that are not elegant (or, at the very least, attempting to be elegant) is never going to be the RN core team's M.O., and is not the correct solution to having 192 open PR's. Large numbers of open PRs is not a reason to start accepting them willy-nilly. I hope that in the coming weeks and months, you see that the core team is trying hard and succeeding at decreasing the number of open issues and PRs. In an effort to keep this discussion related to the PR at hand, if you'd like to continue this discussion, please don't hesitate to post on the React Native Facebook Group (https://www.facebook.com/groups/react.native.community/) or email me personally (adam@sk3vy.com). |
|
@nicklockwood =) As @skevy suggested, I'll take this discussion off of this thread. Apologies for derailing. |
|
@jaygarcia I appreciate your frustration, and in fact, the criterion I use for merging is not actually "is this the ideal solution". As I was discussing with @mkonicek recently, my criterion for merging is more like "does this bring us closer to the ideal solution?". What I mean by that is, will merging this PR make it easier or harder to implement the ideal solution later? If we merge something, people will use it, which makes it harder to change it later. We inevitably do have to make breaking changes occasionally, but we go out of our way to avoid doing so unless it's absolutely necessary because it creates more work for us, and more frustration for users, so adding APIs which we know we're going to have to break is something we try not to do. We get a lot of PRs, and they vary between very small and very complex changes. It's self-evident why the complex ones take a long time to review, but even the very small, simple ones are often fixing a very specific problem in a non-extensible way - for example, #1355 seems like a very simple PR to review because it just adds one property, but in fact:
The answer to each of these questions would imply a different API - maybe it needs to be an array of whitelist URLs instead of a boolean property? Or maybe it needs to be a synchronous callback function to JS? Or maybe we should just implement a custom protocol like "safari://" to mean open links in Safari, and "chrome://" to open in Chrome? And so we'd have to deprecate this property in order to extend the API in any useful way, in which case it's probably better just not to add it in the first place. In an ideal world, I would reply to that (and every other) PR with this level of detail and all of these counter-suggestions, and then we'd have a long discussion about the pros and cons of each option. But then I would never have time to do anything else. |
|
Sorry, I didn't see the comment about moving the discussion before I posted. I agree it makes more sense to discuss this in a group. Feel free to copy and/or post a link to my comment on https://www.facebook.com/groups/react.native.community and reply to it there, if you want. |
|
@mschwartz given the discussion above, how would you like to proceed on this PR? |
|
So can someone summarize what needs to be done to get a basic version of this in:
That makes things simpler/easier to merge right? |
|
@mschwartz updated the pull request. |
|
I didn't see this PR and dropped a bunch of time over the weekend reinventing the wheel 🎱 In case it might add any value to this discussion I've published a PR demonstrating how I had solved the same thing. mosesoak/ReactNativeAutoIP#1 It writes a plaintext file with the IP address into the app target instead of adding a plist entry, but I ended up with the same basic approach. I think one thing that might be a little easier to use in my version is just keeping the main settings in the top of AppDelegate so they're easy to override. You never have to open or modify As to the discussion above, I hear the comment that an interim iOS-only solution would still be a vast improvement over the current implementation, which I can vouch is frustrating to use. However, I also get that this touches on some core configurations (port, automated IP and useBundle are all related) that cross over to Android to some degree, so it seems worth figuring out whether this could be put in a more central location, rather than localized on the iOS side. (Someone on the configurable-port PR suggested that top-level settings might live in the start command in the There's a lot of design considerations to parse through here for sure. That said, I do think it would be valuable as an interim step to move forward with some kind of IP detection in the tooling, since you still need to get the IP address into the app using things like Xcode run scripts in the end. Feel free to comment on my version as well, and authors please LMK if there's any interest in porting any of it to this PR. If not, sorry for the noise! :-) |
Reviewed By: frantic Differential Revision: D2613142 fb-gh-sync-id: fadcea3d23825420c0412f2e4d8d51c70b0f08ed
|
Facebook want something specific here - but they have not spelled out in detail what that is or what changes need to made to get this in. Maybe the best way is for them to spell it out in concise words or code and then we don't have to guess :-) |
|
This is what I would like to see from the implementation: #4681 (comment). If you need this functionality now there are a couple of options:
A fantastic example of this is the work @skevy did here to enable Relay for React Native (currently in review): #5084. He researched the scope of the work, which was quite large across three repos, and came up with a coherent solution that's maintainable too. If the scope of auto IP detection seems too big at first how about focusing on a smaller problem? Ex: refactoring WebSocketExecutor so that configuring it cleanly is possible. Or figuring out a robust, cross-platform way to get the build host's IP address and local hostname into the app? |
|
I really am not that bothered about WebSocketExecutor and the remote debugging stuff. I'd rather just be able to use React Native without having to check my IP Address every time I run on device. I actually think setting localhost to be the hostname of my machine (Andrews-MacBook-Pro.local) will pretty much cover it in a lot of my cases. I've never even got an Android app working so I guess that isn't going to be something many people are going to be jumping all over with great enthusiasm and I don't even know if the same pain points exist. |
|
I really don't understand how the people working on apps at facebook don't genuinely tear their hair out experiencing the red screen of doom every time you change from device to simulator and back again. |
|
Maybe useful? On Tuesday, January 19, 2016, Andrew Phillipo notifications@github.com
|
|
Michael - your solution would work if I wasn't working from 10 different locations. Hostname of my machine will probably work. |
|
@mschwartz updated the pull request. |
1 similar comment
|
@mschwartz updated the pull request. |
|
@aphillipo Great suggestion! The simplest solution is often the best. MyMachine.local also works in RCTWebSocketExecutor. |
|
Has this been fixed? |
Automatic IP detection and implementation. See #4245, #4245 (comment).
For most people, they will never have to edit AppDelegate.m to choose method 1 or method 2 or have to hard code IP address. Also, RCTWebSocketExecutor.m is fixed so you can debug on device to your workstation (Chrome, etc.).