New Arch Support#350
Conversation
…o feature/newArch
levibuzolic
left a comment
levibuzolic
left a comment
There was a problem hiding this comment.
Thanks @zaferatli, looks like this needs a bit more work before it can be merged.
| private final JailMonkeyImpl impl; | ||
|
|
||
| public JailMonkeyModule(ReactApplicationContext reactContext) { | ||
| super(reactContext); | ||
| impl = new JailMonkeyImpl(reactContext); // context impl içinde set ediliyor | ||
| } |
There was a problem hiding this comment.
Should JailMonkeyImpl be JailMonkeyModuleImpl?
| // ===== ASYNC METHODS ===== | ||
| @ReactMethod | ||
| public void isDevelopmentSettingsMode(Promise p) { | ||
| impl.isDevelopmentSettingsMode(p); |
There was a problem hiding this comment.
This is a static method, not a class method. It looks like the old-arch implementation hasn't actually been tried.
|
|
||
| public static boolean isOnExternalStorage() { | ||
| if (context == null) return false; | ||
| return isOnExternalStorage(); |
There was a problem hiding this comment.
Looks like an infinite recursion.
| } | ||
|
|
||
| public static boolean trustFall() { | ||
| return false; |
There was a problem hiding this comment.
This might be a regression, it previously would use isJailBroken and canMockLocation
| constants.put("hookDetected", impl.hookDetected()); | ||
| constants.put("canMockLocation", impl.canMockLocation()); | ||
| constants.put("isOnExternalStorage", impl.isOnExternalStorage()); | ||
| constants.put("AdbEnabled", impl.isAdbEnabled()); |
There was a problem hiding this comment.
Are we missing rootedDetectionMethods?
| s.dependency 'React-Core' | ||
| s.source_files = "JailMonkey/**/*.{h,m,mm}" | ||
| s.dependency "React-Core" | ||
| s.dependency "React-RCTFabric" |
There was a problem hiding this comment.
The new arch deps should be wrapped with ENV['RCT_NEW_ARCH_ENABLED']
| import com.facebook.react.bridge.Promise; | ||
| import com.facebook.react.bridge.ReactApplicationContext; | ||
| import java.util.Map; | ||
| import com.facebook.react.bridge.Promise; |
There was a problem hiding this comment.
Dupe
| import com.facebook.react.bridge.Promise; |
There was a problem hiding this comment.
thanks for all comments, its done
There was a problem hiding this comment.
Also I set version to 3.0.0
|
@levibuzolic Do you have any comments, I've plan to create pre-major releaese and test it in production |
|
I think its done I'm merging and will release with canary for a week and after that we release as latest |
|
@zaferatli awesome, thanks for addressing those issues. Looks good. 👍 |
2 participants
#349
#320