Expose way for native modules to modify JSC context #5540
Conversation
|
By analyzing the blame information on this pull request, we identified @jspahrsummers, @lexs and @mkonicek to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
@appden updated the pull request. |
This notification will be fired on the JS thread after a JSContext has been created and setup, but before the JS bundle is loaded. This allows for advanced native modules to add custom functionality (that isn't possible with the existing APIs) into the context that will be available when JS bundle loads.
This method will return a long that represents the C pointer to the JSGlobalContextRef pointer, or will return 0 when there is no JSC context (debugging through a WebSocket on Chrome). This pointer is only useful if passed back through the JNI and used with the JavaScriptCore C API.
This native module callback will be called on the JS thread after a ReactBridge instance has been created, but before the JS bundle is loaded. This allows for advanced native modules to add custom functionality (that isn't possible with the existing APIs) into the context that will be available when the JS bundle loads.
|
@appden updated the pull request. |
|
How does this interact with Chrome debugging? I imagine that your native module wouldn't work in that case? We've tried to avoid exposing details specific to any particular executor in order to give us more flexibility in the future to switch engines, etc. |
|
@spicyj without knowing the specifics my sense is it's likely incompatible with Chrome debugging. That doesn't worry me a whole lot because the JSC debugger is getting better (slated for OS X 10.12) but your point about switching engines to something like V8 is a good one. |
|
The iOS side of this PR looks reasonable to me, although I'd move the notification definition to RCTJSCExecutor.h, since you'd only want to have access to that symbol if you also depend on the JSC being used. |
|
Thanks for taking the time to look at this! @spicyj we have our own crazy solution for Chrome debugging that hopefully won't be needed for very long considering JSC debugging is coming along. I appreciate wanting future flexibility with changing JS engines, but if that ever happens, we can add an API that returns an enum describing the kind of context being returned, or otherwise alter the API so native modules can access the V8 API instead. @javache I'll gladly move the notification to RCTJSCExecutor.h, if you prefer 👍 |
This address feedback from the PR since this notification is directly tied to using JSC.
|
@appden updated the pull request. |
|
Could you explain why you want to expose the JSC context directly. What are you doing against it that you cannot do with the normal bridge? |
|
Yes, why do you "require" a custom object? |
|
The problem here is that you expose an implementation detail of RN (that we use JSC) to native modules. If you'd explain what you actually need the context for we might be able to build an abstraction for this. |
|
@vjeux @sahrens @lexs I wouldn't go so far to say that the JS engine is an implementation detail. It's certainly not an API most native modules should have to deal with, but for Realm to feasibly be exposed into JS, it's necessary we have direct access to the JS engine APIs. If, for instance, React Native switched to V8 on Android, that would not be a major issue for us as long as we continued to have access to the V8 context so we could instead use the V8 version of our JS bindings. I hope that helps to explain why we need this. 😄 |
|
I think that the best way forward here is to get in a meeting together with people involved and figure out what the next steps are, especially given the time constraint. |
This means that a property read or write won't do I/O on the main JS thread but will update the in-memory db. The transaction has to be explicitly committed, correct? |
Since the database is a memory mapped file, I/O is handled at the kernel level (e.g. page faults). Realm is designed to perform very well on the "main" thread (in this case the JS thread) for most use cases, but some methods will optionally be asynchronous, such as for performing expensive queries. It is indeed transactional – all writes must be made inside a write transaction. |
|
To be clear, the one thing you want to accomplish is install native, synchronous hooks to the VM, correct? I think we're going to come up with a better way to do this down the road, probably when we finish the rewrite of the Android bridge in C++. In the meantime, we will ship this, but be aware that it will be deprecated in the next 6 months-year. As such, I want to make sure these functions are properly documented making sure people understand this is a temporary escape hatch until we have a proper API. |
| @@ -82,6 +82,7 @@ private native void initialize( | |||
| public native void callFunction(int moduleId, int methodId, NativeArray arguments); | |||
| public native void invokeCallback(int callbackID, NativeArray arguments); | |||
| public native void setGlobalVariable(String propertyName, String jsonEncodedArgument); | |||
| public native long getJavaScriptContext(); | |||
There was a problem hiding this comment.
Name something convincingly scary like getJavaScriptContextNativePtrExperimental and document that this API will go away soon.
Renamed and documented it to be extra frightening.
|
@appden updated the pull request. |
|
Comments added, @astreet, @nicklockwood is this good to go? Once it's good just shipit :) |
|
@appden updated the pull request. |
|
hold onto your butts (I actually have a feeling this is going to end in an anti-climactic import failure :P) |
|
I guess the github bot can't handle irrelevant chatter |
|
@facebook-github-bot shipit |
|
@appden updated the pull request. |
|
@astreet the bot is currently broken unfortunately, see our internal post. @bestander and @mkonicek are looking into it |
|
@appden updated the pull request. |
1 similar comment
|
@appden updated the pull request. |
|
@facebook-github-bot shipit |
|
should be fixed now |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1560433314281179/int_phab to review. |
|
@vjeux @astreet @mkonicek @nicklockwood would you mind updating us on the status of this merge? |
|
CPP error |
|
@bestander sounds like an issue after merging with master because |
|
I tried to fix this on the imported diff. It should land now. I'll check on On Mon, 15 Feb 2016 18:34 Scott Kyle notifications@github.com wrote:
|
|
This PR also broke an internal class that extended NativeModule, I've fixed that and will monitor. I'll try to make sure this lands tonight. |
|
It landed with e5ba46c. |
|
Thanks @lexs! |
| JSContext *context = strongSelf.context.context; | ||
| RCTInstallJSCProfiler(_bridge, context.JSGlobalContextRef); | ||
|
|
||
| [[NSNotificationCenter defaultCenter] postNotificationName:RCTJavaScriptContextCreatedNotification |
There was a problem hiding this comment.
This is under an #if RCT_DEV so notifications will never fire in production.
There was a problem hiding this comment.
Yep, that was pretty major mistake on my part. I realized that a little while after this was merged. 😞
We at Realm are developing a native module that actually requires using the JavaScriptCore API to expose a custom object into the context. At the moment, we have been using swizzling in Objective-C and a patched version of React Native, but we obviously can't continue to do this when we release.
The new APIs in this PR were made to best suit the existing React Native APIs on each platform. They provide the necessary hooks for advanced native module authors to accomplish what we are doing without any hacks.