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

Allow ReactActivityDelegate subclass to overrride mainComponentName #19814

Closed
wants to merge 1 commit into from
Closed

Conversation

augustl
Copy link
Contributor

@augustl augustl commented Jun 20, 2018

Currently, the final field mMainComponentName is used. This field is
initialized in the constructor of ReactActivityDelegate, and the
ReactActivityDelegate itself is initialized in the constructor of
ReactActivity. This means that the only way you can pass a main
component name to ReactActivityDelegate, is when your ReactActivity
subclass is constructed. At this point in the lifecycle of an
activity, the getIntent() value that the activity was initialized by
returns null, making it impossible to set the mainComponentName
dynamically based on data passed to the activity via an intent.

The mMainComponentName final field is also only used in onCreate of
the delegate, so it's not actually needed by the ReactActivityDelegate
at construction time. So the above limitation is not fundamental, it's
just a side effect of the API design.

By allowing subclasses of ReactActivityDelegate to implement a
getMainComponentName method, the subclass then has full control of how
to initialize the value. So an implementation of getMainComponentName
could be:

public String getMainComponentName() {
    return getIntent().getStringExtra("reactMainComponentName");
}

This commit doesn't remove anything and only adds a new method, so it
should be fully backwards compatible.

Thank you for sending the PR! We appreciate you spending the time to work on these changes.
Help us understand your motivation by explaining why you decided to make this change.

Test Plan

Related PRs

Release Notes

[ANDROID] [ENHANCEMENT] [ReactActivityDelegate] - Added ability for subclasses to override component name

Changelog:

[Android] [Enhancement] - Added ability for subclasses of ReactActivityDelegate to override component name

@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 Jun 20, 2018
@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jun 20, 2018
@facebook-github-bot
Copy link
Contributor

@augustl I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@augustl I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

Currently, the final field mMainComponentName is used. This field is
initialized in the constructor of ReactActivityDelegate, and the
ReactActivityDelegate itself is initialized in the constructor of
ReactActivity. This means that the only way you can pass a main
component name to ReactActivityDelegate, is when your ReactActivity
subclass is constructed. At this point in the lifecycle of an
activity, the getIntent() value that the activity was initialized by
returns null, making it impossible to set the mainComponentName
dynamically based on data passed to the activity via an intent.

The mMainComponentName final field is also only used in onCreate of
the delegate, so it's not actually needed by the ReactActivityDelegate
at construction time. So the above limitation is not fundamental, it's
just a side effect of the API design.

By allowing subclasses of ReactActivityDelegate to implement a
getMainComponentName method, the subclass then has full control of how
to initialize the value. So an implementation of getMainComponentName
could be:

    public String getMainComponentName() {
        return getIntent().getStringExtra("reactMainComponentName");
    }

This commit doesn't remove anything and only adds a new method, so it
should be fully backwards compatible.
@pull-bot
Copy link

Warnings
⚠️

📋 Changelog - This PR appears to be missing Changelog.

Generated by 🚫 dangerJS

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. Sorry for the long time until we got back to you about this PR.

@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 Feb 25, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @augustl in a8449c1.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 25, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 25, 2019
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. Merged This PR has been merged. Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants