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

[Discussion] Consider adhering to Node-style callbacks or Promises for async NativeModule methods #172

Closed
ide opened this issue Mar 20, 2015 · 5 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@ide
Copy link
Contributor

ide commented Mar 20, 2015

The NativeModule methods that take callbacks currently expect to receive an error handler and a success handler. I believe it would be good if they adhered to a de facto convention.

Promises would probably be best since they work with existing promise libraries and ES7 async/await. Since NativeModule methods are async anyway this makes sense to me. Node-style callbacks ((err, result) => ...) are OK too and has similar benefits around being able to use existing helper libraries.

@vjeux
Copy link
Contributor

vjeux commented Mar 20, 2015

Yeah, we're not really fan of the current state of the world but it's working and haven't been a high enough priority so far.

I really like promises API for that, but I'm a little worried that they run in a setTimeout. Maybe we can just create a fake promise api that just calls then and catch directly.

Tangential to that, we need to figure a better way to represent the error object. Right now it's free for all. Would also be nice to have a stack trace attached.

I'd like to get @sahrens, @jordwalke, @frantic opinion but if they are alright, I'd love for you to promisify the bridge :)

@sahrens
Copy link
Contributor

sahrens commented Mar 20, 2015

Real error objects would be great. AsyncStorage returns pseudo error objects and the JS facad turns them into proper errors. Would be great if our modules were consistent.

-Spencer

On Mar 20, 2015, at 12:22 PM, Christopher Chedeau notifications@github.com wrote:

Yeah, we're not really fan of the current state of the world but it's working and haven't been a high enough priority so far.

I really like promises API for that, but I'm a little worried that they run in a setTimeout. Maybe we can just create a fake promise api that just calls then and catch directly.

Tangential to that, we need to figure a better way to represent the error object. Right now it's free for all. Would also be nice to have a stack trace attached.

I'd like to get @sahrens, @jordwalke, @frantic opinion but if they are alright, I'd love you to promisify the bridge :)


Reply to this email directly or view it on GitHub.

@frantic
Copy link
Contributor

frantic commented Mar 20, 2015

Also note that because we control the bridge we can have async stacktraces support easily, so I wouldn't say setTimeout/setImmediate usage is a concern. Imho the biggest disadvantage of promises is that it's really easy to forget to catch the error.

@ide
Copy link
Contributor Author

ide commented Mar 20, 2015

Imho the biggest disadvantage of promises is that it's really easy to forget to catch the error.

Yeah. I think promises are ok and versatile in some scenarios but not always the API I'd directly want to use. The reason I lean towards them is that they're going to be the lingua franca of asynchrony in JS so to speak, and it will be easy to use promises with callback APIs / promise libraries / ES7 async functions.

@brentvatne brentvatne changed the title Consider adhering to Node-style callbacks or Promises for async NativeModule methods [Discussion] Consider adhering to Node-style callbacks or Promises for async NativeModule methods May 31, 2015
@ide
Copy link
Contributor Author

ide commented Jun 12, 2015

Fixed by #1232 / 90439ce. RCT_EXPORT_METHOD now can export JavaScript functions that return promises and are compatible with async/await.

@ide ide closed this as completed Jun 12, 2015
harrykiselev pushed a commit to harrykiselev/react-native that referenced this issue Aug 5, 2015
keithnorm added a commit to bjornco/react-native that referenced this issue Feb 12, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Feb 22, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Feb 22, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Mar 8, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Mar 9, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Mar 18, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue Apr 14, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
drtangible pushed a commit to bjornco/react-native that referenced this issue May 18, 2016
…ut off on some TextView's.

It seems that the height reported via Layout and height expected based on lineHeight differ for single line text. This was the best workaround I could come up with after working on it for a day.
@facebook facebook locked as resolved and limited conversation to collaborators Jun 12, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants