-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
decouple jsc when USE_THIRD_PARTY_JSC=1 on ios #49371
base: main
Are you sure you want to change the base?
Conversation
4f00ef7
to
ce34e0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me. I left a couple of minor suggestion to avoid code duplication.
packages/react-native/Libraries/AppDelegate/RCTAppSetupUtils.mm
Outdated
Show resolved
Hide resolved
packages/react-native/Libraries/AppDelegate/RCTAppSetupUtils.mm
Outdated
Show resolved
Hide resolved
if ENV['USE_THIRD_PARTY_JSC'] != '1' | ||
pod 'React-jsc', :path => "#{react_native_path}/ReactCommon/jsc" | ||
if fabric_enabled | ||
pod 'React-jsc/Fabric', :path => "#{react_native_path}/ReactCommon/jsc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not even sure that this subspec is actually needed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not but that feels out of scope toward this pr. maybe we can update that in a separate pr?
--------- Co-authored-by: Riccardo Cipolleschi <cipolleschi@meta.com>
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
an effort of lean core for jsc: https://github.com/Kudo/discussions-and-proposals/blob/%40kudo/lean-core-jsc/proposals/0836-lean-core-jsc.md. this pr tries to decouple all jsc code when
USE_THIRD_PARTY_JSC=1
on iosthis pr includes these changes:
React-jsc
pod and pod dependency whenUSE_THIRD_PARTY_JSC=1
JSCExecutorFactory
/RCTJscInstance
references whenUSE_THIRD_PARTY_JSC=1
. it throws c++ errors likeNo JSRuntimeFactory specified.
when no engine is specified (USE_HERMES=0 && USE_THIRD_PARTY_JSC=1). people need to override delegate methods to specify a JSRuntimeFactory.Changelog:
[IOS] [CHANGED] - Decouple JSC when
USE_THIRD_PARTY_JSC=1
Test Plan:
RCT_NEW_ARCH_ENABLED=1 USE_THIRD_PARTY_JSC=1 USE_HERMES=0 USE_FRAMEWORKS=dynamic bundle exec pod install
RCT_NEW_ARCH_ENABLED=0 USE_THIRD_PARTY_JSC=1 USE_HERMES=0 USE_FRAMEWORKS=dynamic bundle exec pod install
RCT_NEW_ARCH_ENABLED=0 USE_THIRD_PARTY_JSC=1 USE_HERMES=0 bundle exec pod install