Update from upstream #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Our Blob implementation was very problematic because it didn't release its underlying resource when the JS instance was dealocated. The main issue is that the fetch polyfill uses blobs by default if the module is available, which causes large memory leaks. This fixes it by using the new jsi infra to attach a `jsi::HostObject` (`BlobCollector`) to `Blob` instances. This way when the `Blob` is collected, the `BlobCollector` also gets collected. Using the `jsi::HostObject` dtor we can schedule the cleanup of native resources. This is definitely not the ideal solution but otherwise it would require rewriting the whole module using TurboModules + jsi. Fixes #23801, #20352, #21092 [General] [Fixed] - [Blob] Release underlying resources when JS instance in GC'ed Pull Request resolved: #24745 Reviewed By: fkgozali Differential Revision: D15248848 Pulled By: hramos fbshipit-source-id: 1da835cc935dfbf4e7bb6fbf2aea29bfdc9bd6fa
Summary: Upgrades React Native's Metro dependencies to 0.54.0. Reviewed By: alexeylang Differential Revision: D15212988 fbshipit-source-id: 5799740bc8df1b778630bba063be17d8739cbef8
Summary:
Core React/Litho support and Java codegen. View updating still in progress.
Generated ViewManager code:
```
// Copyright 2004-present Facebook. All Rights Reserved.
//
// Autogenerated by ComponentsReactNativeSupportProcessor
package com.facebook.catalyst.samples.componentsembedding;
import com.facebook.litho.Component;
import com.facebook.litho.ComponentContext;
import com.facebook.litho.reactnative.ComponentsShadowNode;
import com.facebook.litho.reactnative.ComponentsViewManager;
import com.facebook.litho.reactnative.ReactLithoView;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.uimanager.ReactStylesDiffMap;
import com.facebook.react.uimanager.StateWrapper;
import com.facebook.react.uimanager.ThemedReactContext;
import java.util.HashMap;
import java.util.Map;
ReactModule(
name = "SampleComponent"
)
public class GeneratedSampleComponentViewManager extends ComponentsViewManager {
Override
public String getName() {
return "SampleComponent";
}
Override
public ReactLithoView createViewInstanceWithProps(ThemedReactContext context,
ReactStylesDiffMap props) {
Map<String, Object> propsMap;
if (props != null && props.toMap() != null) {
propsMap = props.toMap();
} else {
// Non-Fabric will always follow this path, initial props are never provided;
return ReactLithoView.create(context, null, null);
}
Component component = createComponentInstanceWithPropsMap(new ComponentContext(context), propsMap);
return ReactLithoView.create(context, component, propsMap);
}
Override
public Component createComponentInstanceWithPropsMap(ComponentContext context,
Map<String, Object> propsMap) {
SampleComponent.Builder componentBuilder = SampleComponent.create(context);
setComponentBuilderPropsWithMap(componentBuilder, propsMap);
return componentBuilder.build();
}
Override
public void updateProperties(ReactLithoView view, ReactStylesDiffMap props) {
super.updateProperties(view, props);
Map<String, Object> propsMap = props.toMap();
Map<String, Object> oldProps = view.getProps();
if (oldProps == null) {
// Non-Fabric RN will always terminate here; we do not store props, nor do we need to.;
// Prop updating happens incrementally in non-Fabric RN.;
return;
}
Map<String, Object> mergedProps = new HashMap<>(oldProps);
mergedProps.putAll(propsMap);
SampleComponent.Builder componentBuilder = SampleComponent.create(new ComponentContext(view.getComponentContext()));
setComponentBuilderPropsWithMap(componentBuilder, mergedProps);
view.setProps(mergedProps);
view.setComponent(componentBuilder.build());
}
private void setComponentBuilderPropsWithMap(SampleComponent.Builder componentBuilder,
Map<String, Object> propsMap) {
if (propsMap != null && propsMap.containsKey("title")) {
componentBuilder.title((((String)propsMap.getOrDefault("title", null))));
}
if (propsMap != null && propsMap.containsKey("imageUri")) {
componentBuilder.imageUri(android.net.Uri.parse(((String)propsMap.getOrDefault("imageUri", null))));
}
}
Override
public GeneratedSampleComponentShadowNode createShadowNodeInstance(ReactApplicationContext context) {
return new GeneratedSampleComponentShadowNode();
}
Override
public Class<? extends ComponentsShadowNode> getShadowNodeClass() {
return GeneratedSampleComponentShadowNode.class;
}
Override
public void updateState(ReactLithoView view, StateWrapper stateWrapper) {
view.setStateWrapper(stateWrapper);
}
}
```
Reviewed By: shergin
Differential Revision: D14846423
fbshipit-source-id: 4eeeb991f7e32c0cec5e9307d6175b81c8fd034e
Summary: Continuation of #24687 > Issue: [Polish the "new app screen"](react-native-community/discussions-and-proposals#122) > This is the pull request for the new intro screen proposal in react native as directed by cpojer This PR was created because the previous one could not be pushed to for some reason. I cleaned up a few small things and added the component as an example to RNTester so we can keep iterating. My plan is to land this, and then polish it and make it the default in a follow-up. [General][Added] - New Intro screen, Icons Removed Lottie Integration 100% React Native 💥 Pull Request resolved: #24737 Differential Revision: D15259092 Pulled By: cpojer fbshipit-source-id: bc141fb1425cf354f29deffd907c37f83fd92c75
Summary: See e94b116 which broke RNTester in open source. Environment variables are always strings, so "0" is a truthy value. This fixes it. [iOS] [fixed] - Fixed broken RNTester Pull Request resolved: #24736 Differential Revision: D15240810 Pulled By: hramos fbshipit-source-id: 76f498222b6ac05131b3820d8356bfee696f46b8
Differential Revision: D15240810 Original commit changeset: 76f498222b6a fbshipit-source-id: c6bb1bb0738609b36faccbee93a32e76d17cde87
Summary: Apparently, Yoga has a "quirks mode" and we have to enable that for Fabric. Which is probably a mistake that we have to make one more time. Reviewed By: davidaurelio Differential Revision: D15267852 fbshipit-source-id: 88a910fafc9ff64fb19376f4b74a2f2fd5827eba
Summary: add nonnull/nullable for swift [iOS] [Changed] - add nonnull/nullable for swift Pull Request resolved: #24729 Differential Revision: D15273046 Pulled By: shergin fbshipit-source-id: 5603007e5aa01d54064da349666a4e4d998b5471
Summary: See e94b116 which broke RNTester in open source. Environment variables are always strings, so "0" is a truthy value. This fixes it. [iOS] [fixed] - Fixed broken RNTester Pull Request resolved: #24736 Reviewed By: fkgozali Differential Revision: D15272951 Pulled By: hramos fbshipit-source-id: aa78f229e05cb553f75cdf6fb4f926829e20a557
…f labels not showing up properly Summary: Reverting cb7e26a to see if this fixes the accessibility issue we are seeing in text inputs. Reviewed By: shergin Differential Revision: D15271883 fbshipit-source-id: e956f93af539e146ac5a7948fdae020c99a1301e
Summary: @public Reduces measure cache size to a number that is enough for 95% of nodes, according to our (FB-internal) measurements. Node size: 776b -> 584b Reviewed By: SidharthGuglani Differential Revision: D15183567 fbshipit-source-id: 9ae8cc78074271a015e7618b931ba0356de87a0c
Summary: Eliminate the regression template, as it is hardly used and it is already served by the bug report template. Triagers can add the Regression label selectively, or we can do so through the bot. Eliminate the discussion template, as it is not meant to be used and we can point people in the right direction using the existing questions template. Rename the document bug template, and point people to the website repo. Update the bug report template, and be less prescriptive about how an issue is reported. As long as the issue is well-described and the steps to repro can be reasonably followed by someone who is investigating the bug, we'll be happy. [Internal] [Changed] - Consolidated issue templates. Pull Request resolved: #24765 Differential Revision: D15276866 Pulled By: cpojer fbshipit-source-id: d99a174982d2454d8cad3a195dfd627a07438611
Summary: @public Publish two events, `NodeAllocation` and `NodeDeallocation`, in the same places where the global node counter is changed. Reviewed By: SidharthGuglani Differential Revision: D15174858 fbshipit-source-id: 6e4e9add88513b9e987189ca5035d76da2a1de55
Summary: @public Test utility on top of the new event system that maintains a counter of instantiated nodes. Meant to replace the global node counter. Reviewed By: SidharthGuglani Differential Revision: D15174855 fbshipit-source-id: 6998472f95a09b8da652257a26596164bdcf43d6
Summary: @public `YGNodeGetInstanceCount` was only ever meant for tests, and caused data races in multi-threaded environments. It was completely replaced with event-based counting for tests. Here we remove public API around the counter, and the counter itself. Reviewed By: SidharthGuglani Differential Revision: D15174857 fbshipit-source-id: 228e85da565bac9e8485121e956a2e41910b11c8
Summary: @public Publish an event when visiting nodes during layouts. Reviewed By: SidharthGuglani Differential Revision: D15206965 fbshipit-source-id: c201f084b1d4186bc64560b8033be965f2549236
Summary: Issue #22234 includes a number of people (myself included) suffering with duplicate resource errors while building in Android. We have been collectively using a patch there but I don't believe any attempt has been made to PR it upstream. I am not excellent at gradle, so I may have approached this in completely the wrong way, but it works for me in the standard init templates, as well as my current work project which has a complicated 2 buildType + 4 productFlavor configuration. If there is a better way to achieve the result I am happy to learn The approach here is to determine the generated path the resources land in, then move them into the main build output tree. If they remain in the generated path android merging fails with duplicate resource errors, so that move (vs copy) is important. [Android] [Fixed] - Fix duplicate resource error in Android build Pull Request resolved: #24518 Differential Revision: D15276981 Pulled By: cpojer fbshipit-source-id: 3fe8c8556e4dcdac5f96a2d4658ac9b5d9b67379
Summary:
If `$buildDir/generated/res/react/${flavorPathSegment}release/raw` contains files during `gradle assembleRelease` script will fail with `Error: Duplicate resources` error.
This patch is based on this issue [22234](#22234) and pull request [24518](#24518).
[Android] [Fixed] - Fix duplicate resource error for raw folder in Android build
[CC from Mike Hardy PR]
Reports of success on the linked issue via use of the patch + patch-package for a couple months, I personally use it full time with all gradle builds (./gradlew clean assembleRelease or if you have a 'staging' flavor, e.g. ./gradlew clean assembleStagingRelease)
Related reading, also cross-links with the linked issue here:
https://stackoverflow.com/questions/53239705/react-native-error-duplicate-resources-android
Pull Request resolved: #24778
Differential Revision: D15277766
Pulled By: cpojer
fbshipit-source-id: 0ccd76d2aa2e13f7c8bfac07d4ef23b74945807a
Summary: Moved events files to yoga/events for better code structure Reviewed By: davidaurelio Differential Revision: D15198566 fbshipit-source-id: 74d451011841d59fae5a1c637f9c33a7d2d1f87e
…nts (#24693) Summary: Fixes an issue introduced in #15894 that can cause events to be dispatched out of order. Also reverted `viewTag` moved to optional property as it didn't actually work and makes code more complex. [iOS] [Fixed] - Fix event ordering when combining coalescable and non-coalescable events Pull Request resolved: #24693 Differential Revision: D15279513 Pulled By: shergin fbshipit-source-id: 3c64aba6d644ea9564572e6de8330b59b51cf4a9
…TBridge Summary: RCTBridge does not need to retain RCTSurfacePresenter, so we enforce that using `OBJC_ASSOCIATION_ASSIGN`. Reviewed By: mdvacca Differential Revision: D15273325 fbshipit-source-id: f223192ff5f781d9e905b004907739a36882bb63
Summary: This diff changes the condition in `ViewShadowNode::isLayoutOnly()` removing checking for `onLayout` event listener. We needed that before because the mechanism of emitting events was based on analyzing mutation instructures (we needed those to be generated for nodes with `onLayout`). Recenly we changed that algorithm deeply integrating in layout infra, so we don't need this anymore. Reviewed By: JoshuaGross, mdvacca Differential Revision: D15273491 fbshipit-source-id: 2d43f00d1b87369d5993fe5ba70c2de36b8ce0c5
Summary: Right now TurboModuleManager gets the JSCallInvokerHolder from the bridge in its constructor; this diff changes the constructor to make the JSCallInvokerHolder a required argument so that TurboModuleManager doesn't directly depend on the bridge. Reviewed By: axe-fb, RSNara Differential Revision: D15227184 fbshipit-source-id: b16e6abaa727587986a132d0e124163acdf55408
Summary: UITextView is accessible by default (some nested views are) and disabling that is not supported. The problem happened because JS side sets `isAccessible` flag for UITextView and UITextField to `true` (with good intent!), which actually disables (surprise!) bult-in accessibility of TextInput on iOS. On iOS accessible elements cannot be nested, so enabling accessibily for some container view (even in a case where this is view is a public API of TextInput on iOS) shadows some features implemented inside the component. (Disabling accessibility of TextInput via `accessible=false` was never supported.) Reviewed By: JoshuaGross Differential Revision: D15280667 fbshipit-source-id: 72509b40383db6ef66c4263bd920f5ee56a42fc1
Summary: Related to #24760 and #24737 This simplifies some styling within the components used for the New App Screen to help advocate for best practices when styling with CSS-like styles in React Native, as well as using React Native's own unique components. There's a bit more detail in each commit. Let me know if you'd like me to pull that info out into the PR description here! [General] [Changed] - Polished up new app screen component styling Pull Request resolved: #24783 Differential Revision: D15284851 Pulled By: cpojer fbshipit-source-id: 954db00d39fc0082bbd4dc96afa7d38dfb7f67d5
Summary: The particular meaningful piece of code that diff removes was added as (successful) attempt to fix an issue which was lately fixed by D14868547. Reviewed By: mdvacca Differential Revision: D15242046 fbshipit-source-id: 88a3e3c629d7c5f84c402b03e45034644147fec4
…iner Summary: ImageLoader is an actual external dependency, not a ImageManager. That change allows to remove dependency on ImageManager from SurfacePresenter and make some other code simpler. Reviewed By: mdvacca Differential Revision: D15242047 fbshipit-source-id: 8622d15b8fdb5c3a7e25091adf7be1108f87ecd5
…location infra Summary: Now it's implementled differently (see -[RCTComponentViewRegistry preallocateViewComponents]), so this code is not being used. Reviewed By: mdvacca Differential Revision: D15242045 fbshipit-source-id: c02eceb978cf1eae778f84a73456e7156ccf503b
Summary: Refactoring ReactContext to move message queue initialization into its own function that can be called independently of initializeWithInstance. This allows you to create a ReactContext with message queue threads without a CatalystInstance. Reviewed By: mdvacca Differential Revision: D15246287 fbshipit-source-id: 4b8c53e68112af7eded47d8c31311500cc296dfe
Summary: We're all friends here, right? 💗 Related to conversation in #24783: #24783 (comment) We're just [making React run on phones](https://www.youtube.com/watch?v=xKu4kmVivFs&start=17&end=21), right? [General] [Changed] - Updated new app greeting screen title 💗 Pull Request resolved: #24785 Differential Revision: D15286468 Pulled By: cpojer fbshipit-source-id: eb73d1c870331f03766b237e9ccb3fa952463be0
Summary: Syncing worker requirement mismatches to improve remote build time. Created actions: MEDIUM: 5 Differential Revision: D15510246 fbshipit-source-id: b8454b4acf2810251d2a4a4515fc0ce2c1a2b327
Summary: Fixes an issue that was including the view config native component verification function even when the native component wasn't included (e.g. on android) Reviewed By: mdvacca Differential Revision: D15513535 fbshipit-source-id: 9b615689c0d64757eeb3d66862e5b1902ea79b20
Summary: This removes the JS for ToolbarAndroid from RN and moves it to Ads manager, which has two remaining uses of it. In a follow-up, I will also move the native code. Reviewed By: rickhanlonii Differential Revision: D15469117 fbshipit-source-id: 68c3f89b85cc589a48f2dced183267daa791b53b
Summary: Link to https://github.com/facebook/react-native/wiki/Issues ## Changelog [Internal] [Changed] - Link to Issues wiki Pull Request resolved: #25035 Differential Revision: D15516862 Pulled By: cpojer fbshipit-source-id: 2f70a28685852293f946c0b1128184226d56aaa7
Summary: This PR solves bug #24393 for Android. Allows an app to be opened with an NFC tag and getting the url trough Linking.getInitialURL() ## Changelog [Android] [Fixed] - This branch checks also for `ACTION_NDEF_DISCOVERED` intent matches to set the initialURL Pull Request resolved: #25055 Differential Revision: D15516873 Pulled By: cpojer fbshipit-source-id: e8803738d857a69e1063e926fc3858a416a0b25e
Summary: Changes RNTester, first attempt in the direction of improving the RNTester overall. Related ticket: #24647 Changed the `js` directory of the RNTester to have the following structure: ``` - js - assets - components - examples - types - utils ``` * **assets** _Any images, gifs, and media content_ * **components** _All shared components_ * **examples** _Example View/Components to be rendered by the App_ * **types** _Shared flow types_ * **utils** _Shared utilities_ ## Changelog [General] [Changed] - Update folder structure of RNTester's JS directory. Pull Request resolved: #25013 Differential Revision: D15515773 Pulled By: cpojer fbshipit-source-id: 0e4b6386127f338dca0ffe8c237073be53a9e221
Summary: Original commit changeset: 41200e572ed7 Reviewed By: mdvacca Differential Revision: D15485156 fbshipit-source-id: d0868a03b7186bb33998afc2c99dd85f31c8fef9
Summary:
[Android][Fix] - Fix how we normalize indices
Before, we were incorrectly normalizing indices given pending view deletion in the view hierarchy (namely, using LayoutAnimations)
What we had before (that was wrong):
* Maintained a pendingIndices sparse array for each tag
* For each pendingIndices sparse array we'd keep track of how many views we deleted at each abstract index
* Given an abstract index to delete a view at, we'd consult `pendingIndices` array to sum how many pending deletes we had for indices equal or smaller than and add to abstract index
^ Above algorithm is wrong and you can follow along with the following example to see how.
## The correct approach
Given these operations in this order:
1. {tagsToDelete: [123], indicesToDelete [2]}
2. {tagsToDelete: [124], indicesToDelete [1]}
3. {tagsToDelete: [125], indicesToDelete [2]}
4. {tagsToDelete: [126], indicesToDelete [1]}
The approach we want to be using to calculate normalized indices:
### Step 1: Delete tag 124 at index 2
|Views:|122|123|124|125|126|127|
|Actual Indices:|0|1|2|3|4|5|
|Abstract Indices:|0|1|2|3|4|5|
=> simple, we just mark the view at 2
### Step 2: Delete tag 123 at index 1
View tags and indices:
|Views|122|123|~~124~~|125|126|127|
|Actual indices|0|1|~~2~~|3|4|5|
|Abstract Indices|0|1||2|3|4|
=> again, simple, we can just use the normalized index 1 because no pending deletes affect this operation
### Step 3: Delete tag 126 at index 2
View tags and indices:
|Views|122|~~123~~|~~124~~|125|126|127|
|Actual Indices|0|~~1~~|~~2~~|3|4|5|
|Abstract Indices|0|||1|2|3|
=> Here we want to normalize this index to 4 because we need to account the 2 views that should be skipped
### Step 4: Delete tag 125 at index 1
View tags and indices:
|Views|122|~~123~~|~~124~~|125|~~126~~|127|
|Actual Indices|0|~~1~~|~~2~~|3|~~4~~|5|
|Abstract Indices|0|||1||2|
=> The normalized index should be 3.
This diff updates the function `normalizeIndex` to do the above algorithm by repurposing `pendingIndicesToDelete` to instead be a sparse int array that holds [normalizedIndex]=[tag] pairs
It's required that `pendingIndicesToDelete` is ordered by the normalizedIndex.
Reviewed By: mdvacca
Differential Revision: D15485132
fbshipit-source-id: 43e57dffa807e8ea50fa1650c5dec13a6fded624
Summary: Rick manually created view config in JS for View; adding some missing attributes/events and using this instead of `requireNativeComponent` Reviewed By: rickhanlonii Differential Revision: D15488008 fbshipit-source-id: 48e925ec0ca2aeba9e6cc66edef0b70ee1c94d27
Summary:
In Flow v0.99 we are changing function type annotations to be strict about their
static properties. This causes a small issue with the union of a Subcription and
`() => mixed` function type, where the latter is now understood to possibly have
an `unsubscribe` property with a mixed type.
This causes the following refinement check, which appears lower in this file, to
cause an error in the next version of Flow:
```
if (cleanup) {
if (cleanup.unsubscribe) {
cleanup.unsubscribe(); // <-- error here
}
// ...
}
```
In Flow v0.99, because `() => mixed` statics are now checked, Flow sees that
`cleanup.unsubscribe` might be `mixed`, which passes the conditional but could
cause an exception when called.
I also needed to change JestMockFn to have exact statics, because tests
sometimes use a value with that type in place of the cleanup function. That runs
into the `{} <: {+p?: void}` rule, which is an error. `{||} <: {+p?:void}` is
not an error.
Reviewed By: josephsavona
Differential Revision: D15522655
fbshipit-source-id: 2ae3c9016e2b07abaac79827082d2f8743623eb5
Summary: We currently have two different codepaths for actually rendering a surface with Fabric on iOS and Android: on iOS we use Fabric's `UIManagerBinding.startSurface` to call `AppRegistry.runApplication`, but on Android we don't; instead we use the same codepath as paper, calling `ReactRootView.runApplication`. This diff does a few different things: 1. Unify iOS and Android by removing the `#ifndef` for Android so that we call `startSurface` for both 2. Pass through the JS module name on Android so that this actually works (it currently passes in an empty string) 3. Remove the call to `ReactRootView.runApplication` for Fabric so that we don't end up doing this twice 4. Copy over some logic that we need from `ReactRootView.runApplication` (make sure that root layout specs get updated, and that content appeared gets logged) Reviewed By: mdvacca Differential Revision: D15501666 fbshipit-source-id: 5c96c8cf036261cb99729b1dbdff0f7c09a32d76
Differential Revision: D15488008 Original commit changeset: 48e925ec0ca2 fbshipit-source-id: 4ffa223e636116777c178386b6e966a4f253c30a
Summary: Create structure of C++ side of mapbuffer project Reviewed By: shergin Differential Revision: D15529650 fbshipit-source-id: b563d3fbcfddcf46802ccb202e372233baad123d
Summary: Android is failing on Appveyor. Removing to regain signal on Windows. See https://ci.appveyor.com/project/Facebook/react-native/builds/24869333/job/tnif7yt4x0lfisju for an example. Pull Request resolved: #25066 Differential Revision: D15532362 Pulled By: hramos fbshipit-source-id: afb55297b385d8b7e497d9cf4d26420f9502df44
Summary: We want to phase out usage of config pointers on nodes. Setting configs is no longer needed, as a config is unly used during construction. Here we deprecate the setter, as it is no longer working as it used to (e.g. changing `useWebDefaults` after a node is constructed). Reviewed By: SidharthGuglani Differential Revision: D15416474 fbshipit-source-id: a2cc06cad0c5148cecce056ece5f141b3defe9a9
Summary: This unbreaks an issue at FB. Reviewed By: rickhanlonii Differential Revision: D15536623 fbshipit-source-id: 2d59542330d2b951908adf8b6c5c41ca4232bb07
Summary: Reverting the generated view configs due to a potential issue Reviewed By: mdvacca Differential Revision: D15539319 fbshipit-source-id: bddf923dcfda18bd074196f06610fea8bb4561b4
Summary: Adding flow types for DeviceInfo module and migrating our codebase over to using `DeviceInfo.getConstants()` Reviewed By: fkgozali Differential Revision: D14645744 fbshipit-source-id: e30a060c6dc92938cd1420ba11a1d837c79d1e32
Summary: When code depends on a module from fbjs, its types come from the node_modules directory. This is problematic when Flow is deployed, since we can't codemod the parts that come from node_modules. Reviewed By: dsainati1 Differential Revision: D15547035 fbshipit-source-id: 8794a32e0f5786bcdd80eab98344d4bc623fb674
Reviewed By: dsainati1 Differential Revision: D15541620 fbshipit-source-id: e19795e13d47dca58c5603b308b7cd60ba67ef86
Summary: part of #24875. I again, am not completely sure how the call site here works- appears settings can be directly accessed? ## Changelog [General] [Added] - Add TM spec for Settings Pull Request resolved: #24879 Reviewed By: RSNara Differential Revision: D15543012 Pulled By: fkgozali fbshipit-source-id: a1df3096a2fc5fe8e65d0ed2398912530bd3911a
Summary: Part of #24875, adds a spec for Networking. Since `sendRequest` methods are different for both platforms, I had to create 2 spec files as Flow would merge their definitions even when I added `Platform.OS` check ## Changelog [General] [Added] - TM spec for Networking Pull Request resolved: #24892 Reviewed By: RSNara Differential Revision: D15543067 Pulled By: fkgozali fbshipit-source-id: 2b91114dfa45e7899bbb139656a30a6fd52e31db
jarvisluong
pushed a commit
that referenced
this pull request
May 30, 2019
Summary: In D14571128, we made it so that when a JS object's property was `undefined`, we wouldn't insert that property into the corresponding NSDictionary. Here are two important observations about that diff: 1. ALL JS `null`s were now being converted to `NSNull`, and JS `undefined`s were now being converted to `nil`. 2. If a JS object's property was explicitly `null`, then we'd insert `NSNull` into the corresponding dictionary. Considering that when a property doesn't exist in a `NSDictionary`, property access returns `nil`, I've made it so that if a JS object's property is either `null` or `undefined`, then we simply do not insert it in the corresponding `NSDictionary`. Also, I've reverted #1 and made it so that `undefined` and `null` always map to the ObjC `nil`. This shouldn't unfix the problem that D14571128 was trying to fix. Here's my understanding of the problem that D14571128 was trying to fix (to make sure I'm not breaking something by this diff). This method was invoked from JS. ``` RCT_EXPORT_METHOD(logEvents:(NSDictionary *)events) { RCTAssert(events, @"You should provide events for logger"); [[NSNotificationCenter defaultCenter] postNotificationName:@"FBReactPerfLoggerDidReceiveEventsNotification" object:nil userInfo:@{@"FBReactPerfLoggerUserInfoPerfEventsKey" : [events copy]}]; } ``` The above dispatch calls into this method, which appends `events` into `_pendingJSPerfEvents`. ``` - (void)reactPerfLoggerDidReceiveEvents:(NSNotification *)notification { NSDictionary *events = notification.userInfo[@"FBReactPerfLoggerUserInfoPerfEventsKey"]; if (events) { dispatch_async(_eventQueue, ^{ if (self->_sessionData.perfLoggerFlagId != nil) { if ([self processJSPerfEvents:events]) { [self reportMetricsIfFinished]; } } else { [self->_pendingJSPerfEvents addObject:events]; } }); } } ``` Then, in `_processJSPerfEvents`, we do the following (link: https://fburl.com/tr4wr2a7): ``` NSNumber *actionId = events[@"actionId"]; if (actionId) { self->_sessionData.actionId = actionId; } ``` So, if `undefined` or `null` was passed in as the `actionId` property of the `events` JS object in `FBReactPerformanceLogger logEvents:`, then we'd default the `NSDictionary` to have `NSNull` in the corresponding property. This is bad because we had this line in FBReactWildePerfLogger (link: https://fburl.com/2nsywl2n): `actionId ? [actionId shortValue] : PerfLoggerActions.SUCCESS`. Essentially, this is the same problem that my diff is trying to fix. Reviewed By: fkgozali Differential Revision: D14625287 fbshipit-source-id: c701d4b6172484cee62494256175e8b205b23c73
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for sending the PR! We appreciate you spending the time to work on these changes.
Help us understand your motivation by explaining why you decided to make this change.
If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
Pull requests that expand test coverage are more likely to get reviewed. Add a test case whenever possible!
Test Plan:
Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
Changelog:
Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.
[CATEGORY] [TYPE] - Message