Summary:
Instead of returning a `bool` which gives no information about the cause of the error,
return `void` and throw when there's some error.
Another alternative is returning `std::error_code`, but that's less flexible than throwing, and
this API already supports throwing.
Changelog: [Internal]
Reviewed By: jbower-fb
Differential Revision: D19170033
fbshipit-source-id: 870cd996a1a53c94524455f31765c1da99f57a1d
Summary:
The documentation in two out of the three variants of `Function::callWithThis` incorrectly stated that the `this` value was to be `undefined` instead of set to `jsThis`, which contradicts the point of the `callWithThis` method existing in the first place. This diff fixes that issue.
Changelog: [General] [Fixed] - Fix documentation comments for HermesJS's `Function::callWithThis` method to accurately reflect how `this` is handled.
Reviewed By: jbower-fb
Differential Revision: D19714074
fbshipit-source-id: 123688c1f7e578d6356bec54604fb6e30f06b0b1
Summary:
Adding a `.clang-tidy` to a bunch of dirs under `react-native-github/ReactAndroid` and `react-native-github/ReactCommon`.
I don't want to add a single `.clang-tidy` at the root because we'll need more fine-grained control over what checks are enabled in different parts of the codebase; for example, fabric will and TM will probably have more checks enabled than older parts of the codebase that we're not actively modernizing, and the Hermes team probably wants its own config to be consistent with the rest of their codebase.
Starting off each `.clang-tidy` by only enabling clang-diagnostic; this is just to test that it's working. In the future, we'll work with the community to gradually enable more checks.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D19705749
fbshipit-source-id: 979cea053b645ac4a9790340033bfcfb49ca0f97
Summary:
In my app I have a case where I need to pass a very large string (45MB) between JS and native. This is obviously suboptimal, but… this is where I'm at.
The main bottleneck to doing this turned out to be `jsi`'s `JSStringToSTLString()`, which was extremely slow. In my case, 4.7s to execute. After this change, 204ms.
I don't really know C++, so I'm not sure this code is 100% correct and safe, and I bet it could be done even better by avoiding the extra memory allocation (would shave off another 70ms).
## Changelog
[General] [Changed] - Make JSStringToSTLString 23x faster
Pull Request resolved: https://github.com/facebook/react-native/pull/26955
Reviewed By: shergin
Differential Revision: D19578728
Pulled By: motiz88
fbshipit-source-id: 2fbce83166953ce928f0a6aa36eed710bfe05383
Summary:
This change will keep project consistency.
This change repeat the previous commit 582738bdc8.
That was created by sammy-SC and reviewed by shergin
That changed
* `ASSERT_TRUE` -> `EXPECT_TRUE`
* `ASSERT_NEAR` -> `EXPECT_NEAR`
* `ASSERT_EQ` -> `EXPECT_EQ`
That said
> 1. Replace ASSERT_* with EXPECT_*. Assert is a fatal assertion. Expect is non-fatal assertion. So if assert fails, tests do not continue and therefore provide less information.
>
> 2. Rename tests in `RawPropsTest.cpp` from `ShadowNodeTest` to `RawPropsTest`.
>
> Source: https://github.com/google/googletest/blob/master/googletest/docs/primer.md#basic-assertions
## Changelog
[CATEGORY] [TYPE] - Message
Pull Request resolved: https://github.com/facebook/react-native/pull/27850
Differential Revision: D19568014
Pulled By: shergin
fbshipit-source-id: 7c22cd1c7ec919675e834a060bd5e681d43a8baf
Summary:
The reason for this change is that it is the primary root that we want people to be using and the naming should reflect that.
#nocancel
build-break
overriding_review_checks_triggers_an_audit_and_retroactive_review
Changelog: [Internal]
Oncall Short Name: fbobjc_sheriff
Differential Revision: D19431128
fbshipit-source-id: c7208e20ed0f5f5eb6c2849428c09a6d4af9b6f3
Summary:
This fixes a crash caused by a `JSError` exception not being successfully caught in a different dynamic library from where it was thrown. Since the libraries were compiled with hidden symbols and loaded with `RTLD_LOCAL`, the exception typeinfo becomes unique to each module.
Reading on this subject:
https://gcc.gnu.org/wiki/Visibilityhttps://stackoverflow.com/questions/14268736/symbol-visibility-exceptions-runtime-error
Reviewed By: mhorowitz
Differential Revision: D19343161
fbshipit-source-id: 4eb3bc2576bbcca2c3aef4f52b5a27dfde214c6a
Summary:
It is time to target SDK version 10.0+.
Changelog: [iOS] [Deprecated] - Deprecating support for iOS/tvOS SDK 9.x, 10.0+ is now required
Reviewed By: mdvacca
Differential Revision: D19265731
fbshipit-source-id: 93b6f9e8f61c5b36ff69e80d3f18256aa96cc2c0
Summary:
If building a JSError causes a JSError, this would lead to infinite recursion. This changes the error handling path so the problem is visible to JS. There's also a few related cleanups included, and a new test case.
Changelog: [General] [Fixed] - If Error global is not callable when building an error, jsi will throw a JS exception back to JS code. #158
Reviewed By: tmikov, dulinriley
Differential Revision: D18796269
fbshipit-source-id: 57a45d144fa2ea5e78d18c27d3130611737dda96
Summary:
This parameter used to be useful for a custom format hermes was developing,
but since Hermes now outputs the Chrome format it isn't useful.
Chrome actually disallows prettified JSON, and requires a special version that is faster to parse.
Therefore, `compact` was the only supported mode.
Changelog: [Internal] Remove compact parameter from `createSnapshotToFile`
Reviewed By: willholen
Differential Revision: D17726742
fbshipit-source-id: 6f39af9046dff2f3b4fba822312a9a89c939ed89
Summary:
`xplat` targets add different deps based on what platform the target is being built for.
for anything using `fb_xplat`, we can put all ios supermodules in `fbobjc_labels` and all android sms in `fbandroid_labels`
There's some weirdness with python targets like `thrift_gen` in `/xplat/mobileconfig/tools/generator/gen-py/BUCK` that don't have platform-specific labels because the except_for list for `fbandroid` doesn't need the `fbsource//` prefix (see changes in `/ios/isolation/infra.mobileconfig.sm`)
Changelog: [Internal]
Reviewed By: shergin, joshleibsly
Differential Revision: D17884952
fbshipit-source-id: e245364cf515b75682990094d24f789d53b1f3f5
Summary: Make it possible for a Runtime implementation to provide its own JSON parsing, by making the method on Value call through Runtime, which now has the default implementation.
Reviewed By: tmikov
Differential Revision: D17637395
fbshipit-source-id: b8997f7d1721a7790326417f3abfa86c875923c9
Summary:
Merging react-native-windows to 0.60 - the visual studio compiler seems to take issue with the existing code
## Changelog
[Internal] [Fixed] - Build fix for react-native-windows (MSVC)
Pull Request resolved: https://github.com/facebook/react-native/pull/26462
Test Plan:
No real change, just making compilers happy.
### Side Note
We'll want this change cherry-pickered to RN 0.60 and RN 0.61 so users of react-native-windows dont have to use our fork of react-native.
Reviewed By: mhorowitz
Differential Revision: D17406081
Pulled By: TheSavior
fbshipit-source-id: bc056e1a545c6478fdcbd5645f3a8dea657162c8
Summary:
`getHeapInfo` was creating and return a `jsi::Object`. This object was not being traced
when API tracing was turned on.
This resulted in a bug in synth traces, an object was being acted upon without its creation source
being logged.
In order to prevent this, change the API of `getHeapInfo` to no longer return any objects, or modify other heap
contents.
This is preferable to having `TracingRuntime` attempt to mimic the operations of two different implementations.
## Changelog:
[Internal] [Changed] - Changed `jsi::Instrumentation::getHeapInfo` to use a `std::unordered_map`
Reviewed By: mhorowitz
Differential Revision: D17273235
fbshipit-source-id: f69860dcc524c2cf501746a41dbac20b4db8c456
Summary: Include <cstdarg>, which defines the va_start and va_end macros.
Reviewed By: tmikov
Differential Revision: D17344433
fbshipit-source-id: f51fb53cc6e016eeef1a9270c1f2c468a5edc967
Summary:
As part of the fix for https://github.com/facebook/react-native/issues/25349 I added `s.static_framework = true` to each podspec in repo (see https://github.com/facebook/react-native/pull/25619#discussion_r306993309 for more context).
This was required to ensure the existing conditional compilation with `#if RCT_DEV` and `__has_include` still worked correctly when `use_frameworks!` is enabled.
However, fkgozali pointed out that it would be ideal if we didn't have this requirement as it could make life difficult for third-party libraries.
This removes the requirement by moving `React-DevSupport.podspec` and `React-RCTWebSocket.podspec` into `React-Core.podspec` as subspecs. This means the symbols are present when `React-Core.podspec` is built dynamically so `s.static_framework = true` isn't required.
This means that any `Podfile` that refers to `React-DevSupport` or `React-RCTWebSocket` will need to be updated to avoid errors.
## Changelog
I don't think this needs a changelog entry since its just a refinement of https://github.com/facebook/react-native/pull/25619.
Pull Request resolved: https://github.com/facebook/react-native/pull/25816
Test Plan:
Check `RNTesterPods` still works both with and without `use_frameworks!`:
1. Go to the `RNTester` directory and run `pod install`.
2. Run the tests in `RNTesterPods.xcworkspace` to see that everything still works fine.
3. Uncomment the `use_frameworks!` line at the top of `RNTester/Podfile` and run `pod install` again.
4. Run the tests again and see that it still works with frameworks enabled.
Reviewed By: hramos
Differential Revision: D16495030
Pulled By: fkgozali
fbshipit-source-id: 2708ac9fd20cd04cb0aea61b2e8ab0d931dfb6d5
Summary:
This is my proposal for fixing `use_frameworks!` compatibility without breaking all `<React/*>` imports I outlined in https://github.com/facebook/react-native/pull/25393#issuecomment-508457700. If accepted, it will fix https://github.com/facebook/react-native/issues/25349.
It builds on the changes I made in https://github.com/facebook/react-native/pull/25496 by ensuring each podspec has a unique value for `header_dir` so that framework imports do not conflict. Every podspec which should be included in the `<React/*>` namespace now includes it's headers from `React-Core.podspec`.
The following pods can still be imported with `<React/*>` and so should not have breaking changes: `React-ART`,`React-DevSupport`, `React-CoreModules`, `React-RCTActionSheet`, `React-RCTAnimation`, `React-RCTBlob`, `React-RCTImage`, `React-RCTLinking`, `React-RCTNetwork`, `React-RCTPushNotification`, `React-RCTSettings`, `React-RCTText`, `React-RCTSettings`, `React-RCTVibration`, `React-RCTWebSocket` .
There are still a few breaking changes which I hope will be acceptable:
- `React-Core.podspec` has been moved to the root of the project. Any `Podfile` that references it will need to update the path.
- ~~`React-turbomodule-core`'s headers now live under `<turbomodule/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- ~~`React-turbomodulesamples`'s headers now live under `<turbomodulesamples/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- ~~`React-TypeSaferty`'s headers now live under `<TypeSafety/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511040967.
- ~~`React-jscallinvoker`'s headers now live under `<jscallinvoker/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- Each podspec now uses `s.static_framework = true`. This means that a minimum of CocoaPods 1.5 ([released in April 2018](http://blog.cocoapods.org/CocoaPods-1.5.0/)) is now required. This is needed so that the ` __has_include` conditions can still work when frameworks are enabled.
Still to do:
- ~~Including `React-turbomodule-core` with `use_frameworks!` enabled causes the C++ import failures we saw in https://github.com/facebook/react-native/issues/25349. I'm sure it will be possible to fix this but I need to dig deeper (perhaps a custom modulemap would be needed).~~ Addressed by https://github.com/facebook/react-native/pull/25619/commits/33573511f02f3502a28bad48e085e9a4b8608302.
- I haven't got Fabric working yet. I wonder if it would be acceptable to move Fabric out of the `<React/*>` namespace since it is new? �
## Changelog
[iOS] [Fixed] - Fixed compatibility with CocoaPods frameworks.
Pull Request resolved: https://github.com/facebook/react-native/pull/25619
Test Plan:
### FB
```
buck build catalyst
```
### Sample Project
Everything should work exactly as before, where `use_frameworks!` is not in `Podfile`s. I have a branch on my [sample project](https://github.com/jtreanor/react-native-cocoapods-frameworks) here which has `use_frameworks!` in its `Podfile` to demonstrate this is fixed.
You can see that it works with these steps:
1. `git clone git@github.com:jtreanor/react-native-cocoapods-frameworks.git`
2. `git checkout fix-frameworks-subspecs`
3. `cd ios && pod install`
4. `cd .. && react-native run-ios`
The sample app will build and run successfully. To see that it still works without frameworks, remove `use_frameworks!` from the `Podfile` and do steps 3 and 4 again.
### RNTesterPods
`RNTesterPodsPods` can now work with or without `use_frameworks!`.
1. Go to the `RNTester` directory and run `pod install`.
2. Run the tests in `RNTesterPods.xcworkspace` to see that everything still works fine.
3. Uncomment the `use_frameworks!` line at the top of `RNTester/Podfile` and run `pod install` again.
4. Run the tests again and see that it still works with frameworks enabled.
Reviewed By: PeteTheHeat
Differential Revision: D16465247
Pulled By: PeteTheHeat
fbshipit-source-id: cad837e9cced06d30cc5b372af1c65c7780b9e7a
Summary:
Based on Hermes Issue: https://github.com/facebook/hermes/issues/47
It was not actually a bug in Hermes, but a bug in JSI, assuming that all string property names from
`folly::dynamic` are ASCII.
Now we'll be more intentional and directly state `forUtf8` rather than the implicit ASCII encoding.
Reviewed By: mhorowitz
Differential Revision: D16347857
fbshipit-source-id: 6bcfbf9f918dc0a7a485b88a1b537d6c2dd322cc
Summary: Ran clang-format on all of JSI.
Reviewed By: mhorowitz
Differential Revision: D16347858
fbshipit-source-id: 004afde1944f60a2c0989a7c38d5b3c58587f1cb
Summary:
This is the first step towards fixing https://github.com/facebook/react-native/issues/25349. These are the changes to the podspec to correctly update dependencies and build config that will cause any breaking change for users or libraries.
I am breaking these changes out from https://github.com/facebook/react-native/pull/25393 as suggested by fkgozali in https://github.com/facebook/react-native/pull/25393#issuecomment-508322884.
These are the changes:
- Made C++ headers in `React-Core` private by default so that ObjC files can import the module without failures.
- Reduced the number of `yoga` headers that are exposed for the same reason as above. As far as I can see this doesn't cause issues but we can find another solution if it does.
- Adding some missing dependencies to fix undefined symbols errors.
- Added `DoubleConversion` to `HEADER_SEARCH_PATHS` where it was missing.
## Changelog
[iOS] [Fixed] - Updated podspecs for improved compatibility with different install types.
Pull Request resolved: https://github.com/facebook/react-native/pull/25496
Test Plan:
Everything should work exactly as before. I have a branch on my [sample project](https://github.com/jtreanor/react-native-cocoapods-frameworks) here which points at this branch to show that it is still working `Podfile` to demonstrate this is fixed.
You can see that it works with these steps:
1. `git clone git@github.com:jtreanor/react-native-cocoapods-frameworks.git`
2. `git checkout podspec-updates`
3. `cd ios && pod install`
4. `cd .. && react-native run-ios`
The sample app will build and run successfully.
Reviewed By: mmmulani
Differential Revision: D16167346
Pulled By: fkgozali
fbshipit-source-id: 1917b2f8779cb172362a457fb3fce686c55056d3
Summary:
The instrumentation header only needs the forward declarations for ostream, so
we can use just include `iosfwd`, as suggested by Riley in an earlier diff.
Reviewed By: kodafb
Differential Revision: D16152451
fbshipit-source-id: 2afbc40e623b180dfc5917fc8093ab15bf647968
Summary:
Adding an instrumentation endpoint that allows us to write a snapshot out to an
arbitrary output stream. This is in addition to `createSnapshotFromFile`. I
reserve the right to replace the latter with the former in a later diff.
This is necessary to allow snapshots to be requested over the chrome debugger
protocol. The protocol sends the snapshot over the wire in chunks so we need
to be able to use an output stream that can do the chunking.
Because LLVM types are not available at the JSI layer, we accept a
`std::ostream` reference which we wrap with `llvm::raw_os_ostream` within
Hermes. We should not incur the static initializer cost (or other code bloat)
of using ostreams as a result.
Reviewed By: cwdick
Differential Revision: D16016319
fbshipit-source-id: 2d0b4848fd5cbe9ddee371d856cd8eb19dd80396
Summary:
The function is not annotated with `const` so `plain()` will return a non-const
reference to the undecorated Runtime already. Seems like the const_cast was a
hold-over from a previous iteration.
Reviewed By: mhorowitz
Differential Revision: D16016320
fbshipit-source-id: 3dfa1e9acf2fc5c1ad61c9a8cd27c3c2e42036d3
Summary:
`/*` is the standard throughout open source code. For example, Firefox uses single /*: https://hg.mozilla.org/mozilla-central/file/21d22b2f541258d3d1cf96c7ba5ad73e96e616b5/gfx/ipc/CompositorWidgetVsyncObserver.cpp#l3
In addition, Rust considers `/**` to be a doc comment (similar to Javadoc) and having such a comment at the beginning of the file causes `rustc` to barf.
Note that some JavaScript tooling requires `/**`. This is OK since JavaScript files were not covered by the linter in the first place, but it would be good to have that tooling fixed too.
Reviewed By: zertosh
Differential Revision: D15640366
fbshipit-source-id: b4ed4599071516364d6109720750d6a43304c089
Summary: This adds the testlib.cpp/h files to external JSI. They're in a `test/` subdirectory so that build scripts using globs like `*.cpp` won't include them.
Reviewed By: mhorowitz
Differential Revision: D15582281
fbshipit-source-id: 1785ee5071fcf98e92fbf3a11eddb21fe84b3799
Summary:
PR https://github.com/facebook/react-native/pull/24633 introduced some inconsistency in crash messaging, this PR fix it. Asked by mhorowitz
[General] [Added] - Consistent reporting native module name on crash on native side
Pull Request resolved: https://github.com/facebook/react-native/pull/24704
Differential Revision: D15237424
Pulled By: cpojer
fbshipit-source-id: ded8db45b2a2ec9998ff33fdbecef3f12c19578f
Summary:
After upgrading RN from 0.57 to 0.59.4 we've received a lot of crash reports like `Exception in HostObject::get: <unknown>` with no clue what native module caused the crash. This commit adds native module name on crash in this situations. Related to https://github.com/facebook/react-native/issues/24607.
[General] [Added] - Report native module name on crash when native module has failed to load
Pull Request resolved: https://github.com/facebook/react-native/pull/24633
Differential Revision: D15120225
Pulled By: cpojer
fbshipit-source-id: cf8e3e5953548a58f1d010eb70343da5ee946ae8
Summary: This change synchronizes the JSI API with its upstream, aiming to make this easier to automate in the future.
Reviewed By: mhorowitz
Differential Revision: D14783311
fbshipit-source-id: c180d0f728afbeb87a3e8e7331a39c06e73b907e
Summary: This will simplify updating the JSI API from upstream in the future.
Reviewed By: mhorowitz
Differential Revision: D14762674
fbshipit-source-id: fa4a86f08425943e301da4ef3df9893ebaa1493e
Summary:
This is the couple of hacks I used after I finished #23802 in order to get fabric working on RNTester. This is inspired from prior work by kmagiera.
The goal of this PR is to show others what I’m struggling with, and to eventually merge it sans hacks.
- Yarn Install
- Uncomment the commented out pods in RNTester's pod file
- Open RNTesterPods workspace
- Run App
- this is only for pods, the non-pod RNTester will no longer work until updated with fabric too.
- `SurfaceHostingView` & `SurfaceHostingProxyRootView` both try to start the surface immediately, this leads to a race condition due to the javascript not having loaded yet, the hack here is:
1. Swizzle the `start` method on `RCTFabricSurface` to no-op when called.
2. Add observer for `RCTJavaScriptDidLoadNotification`
3. Call private method `_startAllSurfaces` on `_surfacePresenter` in AppDelegate when we receive `RCTJavaScriptDidLoadNotification`.
[General] [Added] - Use Fabric in RNTester
Pull Request resolved: https://github.com/facebook/react-native/pull/23803
Reviewed By: shergin, mdvacca
Differential Revision: D14450726
Pulled By: fkgozali
fbshipit-source-id: 8ae2d48634fecb60db539aaf0a2c89ba1f572c27
Summary:
Fixes#23459. It is not legal to write the character array of a std::string, and can result in undefined behavior.
[General] [Fixed] - Crash when substring intersects with emoji
Pull Request resolved: https://github.com/facebook/react-native/pull/23609
Differential Revision: D14198159
Pulled By: mdvacca
fbshipit-source-id: 71060b1b99ddab89793c98c09f99ec9974479e62
Summary:
This PR implements the first part of [RFC0004: CocoaPods Support Improvements](https://github.com/react-native-community/discussions-and-proposals/blob/353d44f64957113409eb16f836e23dba396a6b46/proposals/0004-cocoapods-support-improvements.md), splitting the `React.podspec` into separate podspecs to more closely match the structure of Xcode projects.
The new structure aims to have one to one mapping between Xcode projects and podspecs. The only places where we differ from this mapping are:
* `React/React-DevSupport.podspec`: `DevSupport` is a part of `React.xcodeproj`, which corresponds to the `React-Core` pod. However, we can't include it in the `React-Core` pod because `DevSupport` depends on `React-RCTWebSocket`, which depends on `React-Core`. Pods may not have circular dependencies.
* The new pods under `ReactCommon/` don't have a corresponding `xcodeproj` because there are no `xcodproj` files in `ReactCommon/`. Those C++ modules are included in `React.xcodeproj`.
*Next steps (not in scope of this PR):*
- Start submitting the Podspecs to CocoaPods on a deploy (or turn the React Native repo into a spec repo): this is important in order to make the experience nicer for library consumers, so that it's not necessary to specify the local path of each Podspec in `Podfile`, you can just add `pod 'React', <version>`.
- Add `Podfile` to the default project template (I have a PR ready for this, but because of bugs related to subspecs, it's blocked on this PR)
[iOS] [Changed] - Split React.podspec into separate podspecs for each Xcode project
Pull Request resolved: https://github.com/facebook/react-native/pull/23559
Differential Revision: D14179326
Pulled By: cpojer
fbshipit-source-id: 397a9c30b6b5d24f86c790057c71f0d403f56c3d
Riley Dulin
Josh Leibsly
Gabriel Nunes
Emily Janzer
Moti Zilberman
Radek Pietruszewski
Kevin Gozali
Yuki Ueda
Kudo Chien
Scott Kyle
Marc Horowitz
Andres Suarez
Janette Cheng
Adam Ernst
Daniel Andersson
REDMOND\acoates
SachinTotale
James Treanor
Min ho Kim
Ashok Menon
Rain
zhongwuzw
Will Holen
Dmitry Dushkin
Zeyad Salloum
Jacob Bower
ericlewis
Ville Immonen