[Android] Expose ArgumentExtractor to BaseJavaModule subclasses #9931

felipecsl · 2016-09-15T18:12:36Z

Summary

This change allows subclasses of BaseJavaModule to provide a list of ArgumentExtractor factories.
ArgumentExtractor.Factory is a new interface that allows returning a custom ArgumentExtractor for a given type. By doing this, we allow @ReactMethod annotated methods to take arguments of arbitrary types (not just primitives, ReadableMap and ReadableArray).

Motivation

For any non-trivial use of java modules, you'll quickly need to start sending complex objects through the bridge and it becomes very cumbersome and error prone to manually parse them out of a ReadableMap or ReadableArray every time.
This allows for more flexibility and more powerful java modules with little overhead.

Test Plan

I'm still working on the unit test for this, but opening the PR for early feedback.

ghost · 2016-09-15T18:12:55Z

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

ghost · 2016-09-15T18:16:29Z

@felipecsl updated the pull request - view changes

bestander · 2016-09-15T19:06:00Z

Maybe an instrumentation test would be required

facebook-github-bot · 2016-09-25T18:05:50Z

@felipecsl updated the pull request - view changes

ghost · 2016-09-26T02:06:28Z

@felipecsl updated the pull request - view changes

ghost · 2016-09-26T02:07:18Z

@felipecsl updated the pull request - view changes

felipecsl · 2016-09-26T03:45:30Z

alright test added, @bestander PTAL

bestander · 2016-09-26T10:27:46Z

I'll send a note to android people.
Ping me if it takes long.

felipecsl · 2016-09-28T06:18:24Z

friendly ping again 😄

ghost · 2016-09-28T06:18:50Z

@felipecsl updated the pull request - view changes

lexs · 2016-09-28T14:07:18Z

I kinda like this idea but it's a change of how we see the bridge if it can send complex objects, however as you noted we already do this today just not in a graceful way.

Do you have plans to support this on iOS too?

lexs · 2016-09-28T14:09:01Z

ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java

-  static final public String METHOD_TYPE_ASYNC = "async";
-  static final public String METHOD_TYPE_PROMISE= "promise";
+  private static final String METHOD_TYPE_ASYNC = "async";
+  private static final String METHOD_TYPE_PROMISE= "promise";
  static final public String METHOD_TYPE_SYNC = "sync";


can you change this one too? or at least move the public to be at the beginning

lexs · 2016-09-28T14:12:41Z

ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java

+     * to {@link ReactMethod} annotated methods with the given {@code Type}. Return {@code null} if
+     * this factory can't handle the provided type.
+     */
+    public interface Factory {


Why do you use a factory instead of just having a list of ArgumentExtractors?

This allows you to "plug-in" the logic that associates a specific ArgumentExtractor with a Type. This way you wouldn't need the big if/else/else if... chain in buildArgumentExtractors() and could delegate it to the Factories to provide or not an extractor for the provided Type.

lexs · 2016-09-28T14:16:50Z

ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java


-  private static abstract class ArgumentExtractor<T> {
+  public BaseJavaModule(List<? extends ArgumentExtractor.Factory> argumentExtractorFactories) {


I think I'd prefer having this be provided with from a method instead?
Like: List<ArgumentExtractor> getCustomExtractors()

Also define an extra class so we can tell the type from the extractor, something like:

abstract class CustomArgumentExtractor<T> extends ArgumentExtractor { Class<T> getSupportedType(); }

I though about doing this way but I'm really not a fan of having this "inheritance party" behavior. Using constructor arguments is a much more explicit way of letting subclasses know about that functionality and having a hook point for defining that.
I understand if you're not a fan of that style though and I'd be happy to change it

Let's go ahead and change to using the method instead. I realize I feel strongly about this for a few reasons; even if I agree on that inheritance is not always a good solution, however in this case it's really implementing an interface (even if we provide a simplifying base class). The method also gives us more lazy behavior which is something were trying hard to move to, it also matches how this class already works.

facebook-github-bot · 2016-10-03T22:05:49Z

@felipecsl updated the pull request - view changes

felipecsl · 2016-10-03T22:05:54Z

I can look into making this change for iOS too!

astreet · 2016-11-02T16:20:01Z

I'm ok with this change, @javache are you ok with something similar on iOS?

Nits: we haven't been adding <?> throughout the codebase so lets not just do it in this file.

facebook-github-bot · 2016-11-03T17:01:46Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @bestander as a potential reviewer. Could you take a look please or cc someone with more context?

felipecsl · 2016-11-03T17:28:32Z

BTW I don't think I'll have time to make these changes on iOS as well, at least not for now

hramos · 2016-11-11T19:39:29Z

@lexs any further comments?

@felipecsl there's some conflicts that need to be addressed in CatalystNativeJSToJavaParametersTestCase.java

lexs · 2016-11-11T21:56:00Z

Added a comment about using a method instead of inheritance, rebase, and I'll happily merge it (or somebody else can go ahead and do it).

felipecsl · 2016-11-11T21:59:47Z

@lexs ok thanks for your review, I'll rebase it and update later tonight

felipecsl · 2016-11-12T18:40:50Z

@lexs and @astreet PR rebased and updated based on your comments, PTAL

astreet · 2016-11-22T18:38:39Z

Sorry for the delay -- seems reasonable to me!

@facebook-github-bot shipit

facebook-github-bot · 2016-11-22T18:39:05Z

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

facebook-github-bot · 2016-11-23T06:48:11Z

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.

astreet · 2016-11-23T13:38:39Z

.../src/androidTest/java/com/facebook/react/tests/CatalystNativeJSToJavaParametersTestCase.java

@@ -299,9 +305,28 @@ public void testGetTypeFromArray() {
    assertEquals(ReadableType.Null, array.getType(6));
  }

+  public void testCustomTypeViaArgumentExtractor() {
+    mCatalystInstance.getJSModule(TestJSToJavaParametersModule.class).returnCustomType();


Land failed as tests don't compile since TestJSToJavaParametersModule#returnCustomType isn't defined. Make sure to run the integration tests first: ./scripts/run-android-local-integration-tests.sh

hramos · 2017-01-19T21:33:30Z

Ping @felipecsl

mkonicek · 2017-02-02T11:52:35Z

Closing this because of no response for two weeks. But it was accepted, just needs some minor tweaks and rebase. @felipecsl feel free to send a new PR mentioning this one so people know it should be merged.

felipecsl added 2 commits September 15, 2016 11:00

[WIP] Expose ArgumentExtractor

af6586d

Unit tests WIP

de8663c

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 15, 2016

felipecsl changed the title ~~Expose argument extractor~~ [Android] Expose argument extractor to BaseJavaModule subclasses Sep 15, 2016

felipecsl changed the title ~~[Android] Expose argument extractor to BaseJavaModule subclasses~~ [Android] Expose ArgumentExtractor to BaseJavaModule subclasses Sep 15, 2016

Merge branch 'master' into expose-argument-extractor

c911940

ghost 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 Sep 15, 2016

felipecsl added 2 commits September 25, 2016 09:55

Merge branch 'master' into expose-argument-extractor

622b16b

Adds integration test

7ed62c3

ghost 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 Sep 25, 2016

Fix failing tests

8d9d8ca

ghost 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 Sep 26, 2016

ghost 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 Sep 28, 2016

lexs reviewed Sep 28, 2016

View reviewed changes

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 Oct 3, 2016

hramos added GH Review: needs-revision and removed GH Review: review-needed labels Nov 11, 2016

Rebase with master

ff17232

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 12, 2016

Nits

42e37f0

Merge branch 'master' into expose-argument-extractor

d47aa7a

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Nov 22, 2016

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 Nov 23, 2016

astreet suggested changes Nov 23, 2016

View reviewed changes

mkonicek closed this Feb 2, 2017

felipecsl mentioned this pull request Feb 4, 2017

Discussion Episode 7: React Native with Felipe Lima from Airbnb artem-zinnatullin/TheContext-Podcast#55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Expose ArgumentExtractor to BaseJavaModule subclasses #9931

[Android] Expose ArgumentExtractor to BaseJavaModule subclasses #9931

felipecsl commented Sep 15, 2016

ghost commented Sep 15, 2016

ghost commented Sep 15, 2016

bestander commented Sep 15, 2016

facebook-github-bot commented Sep 25, 2016

ghost commented Sep 26, 2016

ghost commented Sep 26, 2016

felipecsl commented Sep 26, 2016

bestander commented Sep 26, 2016 •

edited

Loading

felipecsl commented Sep 28, 2016

ghost commented Sep 28, 2016

lexs commented Sep 28, 2016

lexs Sep 28, 2016

lexs Sep 28, 2016

felipecsl Oct 3, 2016

lexs Sep 28, 2016

felipecsl Oct 3, 2016

lexs Nov 11, 2016

facebook-github-bot commented Oct 3, 2016

felipecsl commented Oct 3, 2016

astreet commented Nov 2, 2016

facebook-github-bot commented Nov 3, 2016

felipecsl commented Nov 3, 2016

hramos commented Nov 11, 2016

lexs commented Nov 11, 2016

felipecsl commented Nov 11, 2016

felipecsl commented Nov 12, 2016

astreet commented Nov 22, 2016

facebook-github-bot commented Nov 22, 2016

facebook-github-bot commented Nov 23, 2016

astreet Nov 23, 2016

hramos commented Jan 19, 2017

mkonicek commented Feb 2, 2017


		private static abstract class ArgumentExtractor<T> {
		public BaseJavaModule(List<? extends ArgumentExtractor.Factory> argumentExtractorFactories) {

[Android] Expose ArgumentExtractor to BaseJavaModule subclasses #9931

[Android] Expose ArgumentExtractor to BaseJavaModule subclasses #9931

Conversation

felipecsl commented Sep 15, 2016

Summary

Motivation

Test Plan

ghost commented Sep 15, 2016

ghost commented Sep 15, 2016

bestander commented Sep 15, 2016

facebook-github-bot commented Sep 25, 2016

ghost commented Sep 26, 2016

ghost commented Sep 26, 2016

felipecsl commented Sep 26, 2016

bestander commented Sep 26, 2016 • edited Loading

felipecsl commented Sep 28, 2016

ghost commented Sep 28, 2016

lexs commented Sep 28, 2016

lexs Sep 28, 2016

Choose a reason for hiding this comment

lexs Sep 28, 2016

Choose a reason for hiding this comment

felipecsl Oct 3, 2016

Choose a reason for hiding this comment

lexs Sep 28, 2016

Choose a reason for hiding this comment

felipecsl Oct 3, 2016

Choose a reason for hiding this comment

lexs Nov 11, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Oct 3, 2016

felipecsl commented Oct 3, 2016

astreet commented Nov 2, 2016

facebook-github-bot commented Nov 3, 2016

felipecsl commented Nov 3, 2016

hramos commented Nov 11, 2016

lexs commented Nov 11, 2016

felipecsl commented Nov 11, 2016

felipecsl commented Nov 12, 2016

astreet commented Nov 22, 2016

facebook-github-bot commented Nov 22, 2016

facebook-github-bot commented Nov 23, 2016

astreet Nov 23, 2016

Choose a reason for hiding this comment

hramos commented Jan 19, 2017

mkonicek commented Feb 2, 2017

bestander commented Sep 26, 2016 •

edited

Loading