New architecture #227

janicduplessis · 2021-12-16T00:43:36Z

Summary

Test Plan

See updated contributing guide on how to run the example app.

Open the hooks example.

a-eid · 2021-12-16T15:04:41Z

for some reason, I'm not able to run the example app.

a-eid · 2021-12-16T15:08:43Z

I'm wondering why we have to use bundler & bundle ?

janicduplessis · 2021-12-16T17:44:11Z

@a-eid I think you need to add sudo to the gem install bundler. It is to make sure everyone is using the same coocapods version. You can also use your globally installed version if you want by just using pod install in example/ios instead.

example/metro.config.js

jacobp100 · 2021-12-17T09:53:50Z

ios/SafeAreaView/RNCSafeAreaContext.mm

+- (NSDictionary *)getConstants
+{
+  UIWindow *window = [[UIApplication sharedApplication] keyWindow];
+  if (@available(iOS 11.0, *)) {


Is iOS 10 still supported?

It prob was when the code was written, but it shouldn't be now so I can clean that up.

ios/SafeAreaView/RNCSafeAreaViewMode.m

jacobp100 · 2021-12-17T09:56:55Z

This is really cool! I've not really had a chance to play with the new architecture. What's the backwards compat like?

janicduplessis · 2021-12-17T15:32:26Z

@jacobp100 It should be fully backwards compatible. The new architecture is actually already used in RN core.

janicduplessis · 2021-12-17T15:33:50Z

Planning to release this as a prerelease soon to confirm backwards compat is still good. The only thing that might change is require a newer version of react native, but using the new architecture will be opt in still for a while.

billouboq · 2021-12-20T19:36:52Z

Out of curiosity, what will be the advantages of using the new architecture with this module ? :)

janicduplessis · 2021-12-20T20:58:00Z

For now I'm not using any new feature from the new architecture, but it will enable using rendering priorities which can be nice to have sync rendering when insets change and avoid flickers.

It does enable more code sharing via c++, but the main goal of this PR is just maintain current features.

troZee · 2021-12-22T10:00:48Z

example/ios/Podfile

+    :hermes_enabled => true,
+    :fabric_enabled => true,
+    :app_path => "#{__dir__}/..",
+    :config_file_dir => "#{__dir__}/../../..",


Suggested change

:config_file_dir => "#{__dir__}/../../..",

:config_file_dir => "#{__dir__}/../..",

If __dir__ equal to PWD command, it will return system/user/path/react-native-xxxx/example/ios and then we should go back to times to root folder, which is react-native-xxxx

Is is correct ?

Ahh I see your point right now

const codegenConfigFileDir = path.join(confifDir, dependency);

So the fileDir will be outside the library and then will point to that dependency. But the problem with that solution is that I have the other rn libraries in this dir and the codegen scans those dir, which are outside the project

As workaround I used a below command:

"preinstall": "cp ../package.json ./codegenConfig/react-native-pager-view && cp -R ../src ./codegenConfig/react-native-pager-view",

And changed the config_file_dir

:config_file_dir => "#{__dir__}/../codegenConfig",

Planning to make some improvements to config_file_dir. We want to be able to pass a direct path to a library, as well as supporting multiple values with an array.

Interesting, that seems like a better workaround than the one I have now 😅

troZee · 2021-12-22T10:50:45Z

example/package.json

+    "react-native": "0.0.0-20211215-2008-f16cbe59e",
+    "react-native-codegen": "^0.0.12",
+    "react-native-gradle-plugin": "^0.0.3",
+    "react-native-safe-area-context": "../"


I was wondering, why it has been added here, but probably I found a explanation for that

the dep is added to package.json

codegen recognize it as a dep

using a custom path the codegen generates needed files

in postInstall phase this dep is. removed from node modules to avoid duplication

Could you confirm, if my above thought are correct ?

Yep that's why, it's a hack to make codegen discover the library properly, while still using the source in the repo with the babel plugin alias to make dev simple.

Not sure if there's a better way to setup that, but for now that works.

sammy-SC

Look great! I just left few small comments. I was focusing on C++ and iOS part.

sammy-SC · 2021-12-30T15:56:42Z

ios/SafeAreaView/Fabric/RNCSafeAreaViewComponentView.mm

+}
+
+- (void)updateState:(facebook::react::State::Shared const &)state
+           oldState:(facebook::react::State::Shared const &)oldState


you can drop facebook::react as there is already using namespace on top of the file.

sammy-SC · 2021-12-30T15:58:14Z

ios/SafeAreaView/Fabric/RNCSafeAreaViewComponentView.mm

+  [self updateState];
+}
+
+- (UIView *)findNearestProvider


Would it be appropriate to log a warning in case a provider is not found?

In the case that there is no provider it will use its own view to get insets from, so I think it is correct to not log anything as it will still work properly.

sammy-SC · 2021-12-30T16:01:00Z

ios/SafeAreaView/Fabric/RNCSafeAreaViewComponentView.mm

+  [self invalidateSafeAreaInsets];
+}
+
+- (void)invalidateSafeAreaInsets


Nit: often methods on iOS have suffix "ifNecessary". It suggests to a reader that there is a condition that needs to be met for the action to happen.

Maybe this method could be named "updateStateIfNecessary"?

sammy-SC · 2021-12-30T16:04:39Z

common/cpp/react/renderer/components/safeareacontext/RNCSafeAreaViewShadowNode.cpp

+  }
+
+  auto currentStyle = yogaNode_.getStyle();
+  if (adjustedStyle.padding()[YGEdgeTop] != currentStyle.padding()[YGEdgeTop] ||


Could we compare adjustedStyle != currentStyle instead of comparing every value independently?

Alternatively, compare specifically only padding and margin instead of writing out all edges.

Comaping paddings / margin leads to this compiler error Invalid operands to binary expression ('IdxRef<YGEdge, &YGStyle::padding_>' and 'IdxRef<YGEdge, &YGStyle::padding_>'). I assume it doesn't implement the proper compare functions in yoga.

sammy-SC · 2021-12-30T16:07:49Z

common/cpp/react/renderer/components/safeareacontext/RNCSafeAreaViewShadowNode.cpp

+    right =
+        valueFromEdges(props.yogaStyle.margin(), YGEdgeRight, YGEdgeHorizontal);
+  }
+  if (std::find(edges.begin(), edges.end(), "top") != edges.end()) {


I think you could do following:

edges.find("top") != edges.end()

same result but it is easier to read.

I think free function std::find is meant to be used in templates.

I don't think find exists for std::vector, am I missing something?

Ideally this would be a bitmask or set, but that's what is generated by codegen so I can't change it.

friederbluemle · 2022-02-18T19:47:21Z

Apologies for possibly using the wrong thread (couldn't find a better place).

The release notes for 3.4.0 released 3 days ago mention

Drop support for ios < 11

Isn't this a breaking change that should be done in a major version update? Reason I'm asking is because we've been getting some errors on CI during CocoaPods installation:

Specs satisfying the `react-native-safe-area-context (from `node_modules/react-native-safe-area-context`)`
dependency were found, but they required a higher minimum deployment target.

janicduplessis · 2022-02-18T20:07:25Z

@friederbluemle I should have made this change as part of 4.0 in hindsight. Do you still need support for iOS < 11? In that case I would recommend staying on 3.3.x, or you can update the ios version in your podfile.

janicduplessis · 2022-02-18T23:48:42Z

I decided to revert the ios min version change, it will be done in v4 instead.

friederbluemle · 2022-02-22T06:18:25Z

Thank you @janicduplessis 👍

jacobp100 reviewed Dec 17, 2021

View reviewed changes

example/metro.config.js Outdated Show resolved Hide resolved

jacobp100 reviewed Dec 17, 2021

View reviewed changes

ios/SafeAreaView/RNCSafeAreaViewMode.m Outdated Show resolved Hide resolved

troZee reviewed Dec 22, 2021

View reviewed changes

sammy-SC reviewed Dec 30, 2021

View reviewed changes

troZee mentioned this pull request Jan 3, 2022

📚 Guide: Getting Started with The New Architecture (a.k.a. the Playbook) facebook/react-native-website#2879

Merged

janicduplessis added 2 commits February 18, 2022 02:56

New architecture

30a1e52

deps

7cec01a

janicduplessis force-pushed the new-arch-v2 branch from c0dded4 to 7cec01a Compare February 18, 2022 08:00

janicduplessis added 2 commits February 18, 2022 03:13

ci + fixes

3903614

Move ios code, fix android

8677b7f

janicduplessis marked this pull request as ready for review February 18, 2022 08:29

fix android build

556297f

janicduplessis merged commit 9dbb086 into main Feb 18, 2022

janicduplessis deleted the new-arch-v2 branch February 18, 2022 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New architecture #227

New architecture #227

janicduplessis commented Dec 16, 2021 •

edited

Loading

a-eid commented Dec 16, 2021

a-eid commented Dec 16, 2021

janicduplessis commented Dec 16, 2021

jacobp100 Dec 17, 2021

janicduplessis Dec 17, 2021

jacobp100 commented Dec 17, 2021

janicduplessis commented Dec 17, 2021

janicduplessis commented Dec 17, 2021

billouboq commented Dec 20, 2021

janicduplessis commented Dec 20, 2021

troZee Dec 22, 2021

troZee Dec 22, 2021 •

edited

Loading

janicduplessis Dec 22, 2021

janicduplessis Dec 22, 2021

troZee Dec 22, 2021

janicduplessis Dec 22, 2021

sammy-SC left a comment

sammy-SC Dec 30, 2021

sammy-SC Dec 30, 2021

janicduplessis Feb 17, 2022

sammy-SC Dec 30, 2021

sammy-SC Dec 30, 2021

janicduplessis Feb 17, 2022

sammy-SC Dec 30, 2021

janicduplessis Feb 17, 2022

friederbluemle commented Feb 18, 2022

janicduplessis commented Feb 18, 2022

janicduplessis commented Feb 18, 2022

friederbluemle commented Feb 22, 2022

	:config_file_dir => "#{__dir__}/../../..",
	:config_file_dir => "#{__dir__}/../..",

New architecture #227

New architecture #227

Conversation

janicduplessis commented Dec 16, 2021 • edited Loading

Summary

Test Plan

a-eid commented Dec 16, 2021

a-eid commented Dec 16, 2021

janicduplessis commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobp100 commented Dec 17, 2021

janicduplessis commented Dec 17, 2021

janicduplessis commented Dec 17, 2021

billouboq commented Dec 20, 2021

janicduplessis commented Dec 20, 2021

Choose a reason for hiding this comment

troZee Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammy-SC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friederbluemle commented Feb 18, 2022

janicduplessis commented Feb 18, 2022

janicduplessis commented Feb 18, 2022

friederbluemle commented Feb 22, 2022

janicduplessis commented Dec 16, 2021 •

edited

Loading

troZee Dec 22, 2021 •

edited

Loading