Skip to content

Conversation

@geovie
Copy link
Contributor

@geovie geovie commented Nov 30, 2018

Provide a way for bindings to call back into JavaScript from threads other than the Node.JS main thread

Rendered

Preliminary implementation: neon-bindings/neon#375

@amilajack
Copy link
Member

@geovie sorry for the delay on this. We'll review this soon

// schedule a js function call with the arguments returned from the closure
pub fn schedule<T, F>(&self, closure: F)
where T: Value
F: FnOnce(&mut TaskContext, Handle<JsValue>, Handle<JsFunction>) -> Vec<Handle<T>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably IntoIterator can be used instead of Vec, but that can be decided during the implementation

Copy link
Member

Choose a reason for hiding this comment

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

It needs to be turned into a Vec for other methods. Making it IntoIterator would be inefficient for the common case of already having a vector.

I really like this idea, but I think for it to make sense everything that takes a Vec<Handle<JsValue>> should be switched to IntoIterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kjvalencik Vec already implements IntoIterator, so it can be passed directly.

@ghost
Copy link

ghost commented Sep 3, 2019

Hi, I would love to see this completed, merged and released, can I help for anything?

@kjvalencik
Copy link
Member

@z0mbie42 this is being discussed both in this RFC as well as in the slack workspace. You are welcome to join the conversation if you have thoughts! We don't currently have an expected completion date, but hopefully not long!

@kjvalencik kjvalencik changed the title RFC: ThreadSafeCallback RFC: EventHandler Sep 5, 2019
@kjvalencik
Copy link
Member

@dherman This looks good to me. Do you want to make a call for final comment period or given the demand for this feature, should we skip that?

@dherman
Copy link
Contributor

dherman commented Sep 5, 2019

I like the simplification and reduction of boilerplate that you get from just being able to compute the arguments list instead of having to explicitly call the handler. In the future if we wanted (say, for performance) a lower-level method that lets you schedule arbitrary logic that take the this and func as explicit parameters and lets you decide whether to call them:

cb.schedule_with(move |cx, this, func| { ... })

we could add that backwards-compatibly. I don't think it's urgent, though, and I think we should accept the RFC as-is. We can always add more methods later.

Awesome work, and thanks for keeping at it!

@dherman dherman added the final comment period last opportunity to discuss the proposed conclusion label Sep 5, 2019
@dherman
Copy link
Contributor

dherman commented Sep 5, 2019

I've labelled the RFC as being in final comment period! Thanks again @geovie for your work on this.

@dherman dherman merged commit d2b1b1b into neon-bindings:master Sep 6, 2019
@dherman
Copy link
Contributor

dherman commented Sep 6, 2019

🎉🎉🎉🎉🎉 RFC #25 is merged! 🎉🎉🎉🎉🎉

dherman added a commit that referenced this pull request Sep 6, 2019
@dherman
Copy link
Contributor

dherman commented Jan 31, 2020

RFC #25 is implemented and merged behind a feature flag in neon-bindings/neon#375!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

final comment period last opportunity to discuss the proposed conclusion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants