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

BREAKING: Android - ReactNativeHost getUseDeveloperSupport to public #11329

Closed

Conversation

jpshelley
Copy link
Contributor

Overview

Currently React Native is opinionated in that the easiest approach is to extend ReactActivity. However to more easily allow integrating with existing application, we should allow some of the methods in ReactNativeHost to be public, and this is a very good first step.

  • There is no harm in making this public from what I can tell.
  • This allows ReactNativeHost to be more easily used outside of the ReactActivity and ReactActivityDelegate ecosystem. (A ReactFragment would be a good example)

Addresses / Changes

No issues found

Test plan (required)

  • Run any sample app and verify it still works.

Make sure tests pass on both Travis and Circle CI.

…DevSupportPublic

* upstream/master:
  Allow configuring the way CLI installs react-native
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @foghina and @AaaChiuuu to be potential reviewers.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Dec 6, 2016
@facebook-github-bot
Copy link
Contributor

@AaaChiuuu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 6, 2016
@jpshelley
Copy link
Contributor Author

Looks like Circle failed, I can't tell how to fix the failure though.

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Dec 7, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Dec 7, 2016

As expected, we have some closed-source code that extends the application that we'll need to update.

Error from internal CI:

getUseDeveloperSupport() in com.facebook.someapp.SomeClass cannot override getUseDeveloperSupport()

@mkonicek
Copy link
Contributor

mkonicek commented Dec 7, 2016

@jpshelley Is this change going to break the build for anyone overriding this method as protected boolean getUseDeveloperSupport() in their apps?

If yes, we need to clearly mark it as a breaking change (just did that).

How common is to override this method? Will this break the build for a lot of people?

@mkonicek mkonicek changed the title Android - ReactNativeHost getUseDeveloperSupport to public BREAKING: Android - ReactNativeHost getUseDeveloperSupport to public Dec 7, 2016
…DevSupportPublic

* upstream/master:
  Rename directories
  Use absolute paths from repo root
  Remove init from api
  CLI: Show npm / yarn output during 'react-native-init' when installing React and Jest
@jpshelley
Copy link
Contributor Author

jpshelley commented Dec 7, 2016

@mkonicek

How common is to override this method?

According to the react native documentation, it will be very common for others to override this.

Whats strange is UIExplorer (3rd link) already has it as public.

Will this break the build for a lot of people?

Yes this will unfortunately. It is a quick fix from changing the scope from protected to public.

* master:
  Implement onViewAppear by creating a new EventListener on ReactRootView listening for when it's attached to a RN Instance
@jpshelley
Copy link
Contributor Author

@mkonicek Is this something we can move forward on still? Like I said, some applications use a single activity multiple fragment approach and this would allow for creating a React Fragment.

jpshelley added a commit to jpshelley/react-native that referenced this pull request Dec 12, 2016
@aaronechiu
Copy link
Contributor

I'm attempting to fix the breakage internally right now; we'll see how it goes.

@jpshelley
Copy link
Contributor Author

@AaaChiuuu it looks like the PR was accepted, however it didn't pick up all the changes. Was this intentional? I fear the example applications and any new ones started via the CLI won't build correctly.

@aaronechiu
Copy link
Contributor

Crap, wanna send a new PR?

@jpshelley
Copy link
Contributor Author

@AaaChiuuu I can, can you not take the commits from this PR somehow?

@aaronechiu
Copy link
Contributor

can't re-import the same PR

facebook-github-bot pushed a commit that referenced this pull request Dec 16, 2016
Summary:
AaaChiuuu
See: #11329
Closes #11505

Differential Revision: D4338559

Pulled By: AaaChiuuu

fbshipit-source-id: 6cd1fd366a2bc30d496b7e907242e7f89a384a19
@jpshelley jpshelley deleted the android-AllowHostDevSupportPublic branch December 16, 2016 15:14
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
Currently React Native is opinionated in that the easiest approach is to extend ReactActivity. However to more easily allow integrating with existing application, we should allow some of the methods in ReactNativeHost to be public, and this is a very good first step.
* There is no harm in making this public from what I can tell.
* This allows `ReactNativeHost` to be more easily used outside of the `ReactActivity` and `ReactActivityDelegate` ecosystem. (A `ReactFragment` would be a good example)

_No issues found_

**Test plan (required)**

* Run any sample app and verify it still works.

Make sure tests pass on both Travis and Circle CI.
Closes facebook#11329

Differential Revision: D4287429

Pulled By: AaaChiuuu

fbshipit-source-id: 8cb76f3226aae3737af5f5bd6010d3eea8df9bfe
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
AaaChiuuu
See: facebook#11329
Closes facebook#11505

Differential Revision: D4338559

Pulled By: AaaChiuuu

fbshipit-source-id: 6cd1fd366a2bc30d496b7e907242e7f89a384a19
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Feb 7, 2017
Summary:
AaaChiuuu
See: facebook/react-native#11329
Closes facebook/react-native#11505

Differential Revision: D4338559

Pulled By: AaaChiuuu

fbshipit-source-id: 6cd1fd366a2bc30d496b7e907242e7f89a384a19
chromakode added a commit to keybase/client that referenced this pull request Feb 10, 2017
drgrey87 added a commit to drgrey87/react-native-router-flux that referenced this pull request Mar 9, 2017
cridenour pushed a commit to aksonov/react-native-router-flux that referenced this pull request Mar 10, 2017
jonohodgson pushed a commit to jonohodgson/react-native-router-flux that referenced this pull request May 5, 2017
…er-flux

# By Pavlo Aksonov (5) and others
# Via Pavlo Aksonov (2) and aksonov (1)
* 'master' of https://github.com/aksonov/react-native-router-flux: (24 commits)
  bump version
  Add selection view to show below navigationBarTitleImage (aksonov#1756)
  Maintain tab parentIndex when jumping tabs (aksonov#1775)
  disable Example tests because of new jest issues
  add 'fade' to direction type (aksonov#1705)
  update documentation for nested routing (aksonov#1742)
  update demo to work with latest React Native
  fixed issue aksonov#1688 (in accordance with facebook/react-native#11329) (aksonov#1691)
  use lodash's isEqual for property comparison (aksonov#1682)
  added icon to SceneProps type (aksonov#1546)
  Adding sizes NavBar for Windows (aksonov#1548)
  Added backAndroidHandler to routerProps typings (aksonov#1586)
  add `tabBarBackgroundImageStyle` prop (aksonov#1611)
  Changes drawer button side according with drawer.props.side (aksonov#1584)
  Fix for panGestures and Animation (aksonov#1618)
  Change back button direction in RTL mode (aksonov#1661)
  Add navBarTitleImage prop (aksonov#1663)
  Revert "pass props" (aksonov#1660)
  pass props (aksonov#1549)
  fix dependencies
  ...

Conflicts:
	src/NavBar.js
esco added a commit to esco/react-native-splash-screen that referenced this pull request May 24, 2018
A breaking change made `getUseDeveloperSupport` public facebook/react-native#11329
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
AaaChiuuu
See: facebook/react-native#11329
Closes facebook/react-native#11505

Differential Revision: D4338559

Pulled By: AaaChiuuu

fbshipit-source-id: 6cd1fd366a2bc30d496b7e907242e7f89a384a19
power1220 added a commit to power1220/react-native-router-flux that referenced this pull request Apr 5, 2023
tigerhunter318 pushed a commit to tigerhunter318/react-native-splash-screen that referenced this pull request Oct 31, 2023
A breaking change made `getUseDeveloperSupport` public facebook/react-native#11329
uradev0123 pushed a commit to uradev0123/react-native-splash-screen that referenced this pull request Nov 16, 2023
A breaking change made `getUseDeveloperSupport` public facebook/react-native#11329
LuckyCoder5 added a commit to LuckyCoder5/react-native-router-flux that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants