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

[TM] Add spec for Linking #24877

Closed

Conversation

ericlewis
Copy link
Contributor

Summary

Part of #24875, adds a spec for linking.

Changelog

[General] [Added] - TM Spec for Linking

Test Plan

Flow passes.

@ericlewis ericlewis requested a review from RSNara May 16, 2019 01:56
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels May 16, 2019
@ericlewis

This comment has been minimized.

@react-native-bot react-native-bot added API: Linking Type: Enhancement A new feature or enhancement of an existing feature. labels May 16, 2019
Libraries/Linking/Linking.js Outdated Show resolved Hide resolved

const moduleName =
Platform.OS === 'ios' ? 'NativeLinkingManager' : 'NativeIntentAndroid';
export default TurboModuleRegistry.getEnforcing<Spec>(moduleName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use a variable for module name, so that scripting can collect these callsites (we have an internal script that find all JS callsites for a TurboModule).

if (Platform.OS === 'ios') {
  export default TurboModuleRegistry.getEnforcing<Spec>('NativeLinkingManager');
} else {
  export default TurboModuleRegistry.getEnforcing<Spec>('NativeIntentAndroid');
}

@RSNara: we probably should rename these modules in the native side to have the same name...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fkgozali btw, it is not legal to export that way.

Copy link
Contributor Author

@ericlewis ericlewis May 16, 2019

Choose a reason for hiding this comment

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

export default (Platform.OS === 'ios'
  ? TurboModuleRegistry.getEnforcing<Spec>('LinkingManager')
  : TurboModuleRegistry.getEnforcing<Spec>('IntentAndroid'));

^ is valid

@fkgozali
Copy link
Contributor

lgtm, landing

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.

@fkgozali 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 @ericlewis in 99899d0.

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 May 16, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Part of facebook#24875, adds a spec for linking.

## Changelog

[General] [Added] - TM Spec for Linking
Pull Request resolved: facebook#24877

Differential Revision: D15374328

Pulled By: fkgozali

fbshipit-source-id: 4b86a75d58d275c0ddc864b4f3f1ec489b0b408b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Linking CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Native Module 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