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

Fix Promise argument assertion to take into account executorTokenOffset. #6633

Closed
wants to merge 1 commit into from

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Mar 24, 2016

In the code that extracts and validates arguments from a @ReactMethod, there is verification that if a method contains a Promise in it's list of arguments that it must come last. This fix makes sure that the executorTokenOffset is taken into account when asserting that condition.

Test Plan: Add a Promise argument to a @ReactMethod annotated method in a module that supports web workers, and observe a successful build.

cc @astreet

@skevy skevy added the Android label Mar 24, 2016
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @davidaurelio, @mkonicek and @lexs to be potential reviewers.

@facebook-github-bot facebook-github-bot 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 Mar 24, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Apr 1, 2016

I'll let @astreet take a look.

@ide
Copy link
Contributor

ide commented Apr 5, 2016

This looks like it does the right thing but I think semantically we want to check if i + executorTokenOffset is at the end. Will update the PR.

@ide ide force-pushed the fix-promise-argument-assertion branch from ee4c03b to c4e04a8 Compare April 5, 2016 20:02
@ide
Copy link
Contributor

ide commented Apr 5, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in e27a27b Apr 6, 2016
@ide ide deleted the fix-promise-argument-assertion branch April 6, 2016 03:15
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:In the code that extracts and validates arguments from a `ReactMethod`, there is verification that if a method contains a Promise in it's list of arguments that it must come last. This fix makes sure that the `executorTokenOffset` is taken into account when asserting that condition.
Closes facebook#6633

Differential Revision: D3143207

fb-gh-sync-id: ae9ebd9d829f88993f9951c4cb2452b3f7618476
fbshipit-source-id: ae9ebd9d829f88993f9951c4cb2452b3f7618476
This pull request was closed.
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.

None yet

5 participants