Skip to content

Conversation

@rectified95
Copy link
Contributor

@rectified95 rectified95 commented May 27, 2021

Few changes this time. Commits facebook/react-native@7e05480...438a4cf

Microsoft Reviewers: Open in CodeFlow

@rectified95 rectified95 requested a review from a team as a code owner May 27, 2021 00:03
@rectified95 rectified95 requested a review from NickGerleman May 27, 2021 00:03
@rectified95 rectified95 changed the title Integrate may 5 Integrate May 5th RN nightly build. May 27, 2021
@NickGerleman
Copy link
Contributor

Looks like a new switch JS component was shipped. Integration tests are having trouble measuring it, but does it work correctly manually?

@asklar
Copy link
Member

asklar commented May 27, 2021

anyone have more context on getViewManagerConfig() being deprecated / should we call this out in changelog?

@NickGerleman
Copy link
Contributor

NickGerleman commented May 27, 2021

Skimming through PRs, I think these are the ones we probably want to validate manually:

Note that we do not care much about facebook/react-native@2b67631 (bumping the hermes-engine npm package) since we control that for Windows via NuGet package version.

Picking these commits is more of an art than a science. It helps when you learn what breaks we see. Some of what we care about here ends up being:

  • Anything at the RNW RN Core boundary
    • E.g. anything directly interacting with a native component
  • Platform-specific instance management code (e.g. Instance, NativeToJSBridge, JSIExecutor)
  • JS Engine or JSI changes
    • Our implementations live in other repos
    • And, we think they kind of have to unless we impacted RNW dev experience
  • JS logic around desktop-specific behavior
    • E.g. anything we forked to add mouse support
  • OSS packages
    • E.g. CLI, Metro
  • Code-generation frontend
    • Backend (e.g. Android) we don't much care about
  • RNTester
    • Mostly just custom examples at this point
  • New usages of unimplemented APIs
    • E.g. if a property becomes needed for more regular operation
  • Anything possibly compiler specific

@NickGerleman
Copy link
Contributor

cc @HeyImChris who will also have some experience from RN macOS integration. Have anything to add to the above list that you've encountered for macOS?

@asklar
Copy link
Member

asklar commented May 27, 2021

meta: curious about how RN breaking changes like these are marked; if they're not, it would be good to establish a process to do so : )

@NickGerleman
Copy link
Contributor

meta: curious about how RN breaking changes like these are marked; if they're not, it would be good to establish a process to do so : )

They aren't really breaking to the general public, so they usually aren't marked in my experience. We are working at what are essentially private API boundaries. There are definitely cases where we can make a cleaner, more public or platform-neutral boundary in upstream code (or push that new changes do that 😉).

@NickGerleman
Copy link
Contributor

@TheSavior I’ve gotten the take that FB internal also has some custom platforms. Are there any tags/labels on internal stacks which alter the paper view manager boundary?

@elicwhite
Copy link

elicwhite commented May 29, 2021

@NickGerleman I'm not sure what you mean, can you elaborate?

For the new Switch component, one of the work streams on our team right now is to make React Native concurrent mode compatible. This has involved rewriting a lot of our components. We have been A/B testing those rewrites in prod to verify neutral metrics before we replace the old implementation.

@NickGerleman
Copy link
Contributor

NickGerleman commented May 29, 2021

If an engineer changes interaction with a native component, do they ensure compat with React VR, or other platforms that are not Android/iOS?

The meta is I think finding way to create signal for changes which requires attention of anyone with native implementations of stock components.

@elicwhite
Copy link

elicwhite commented May 29, 2021

Ah. It depends, but we aren't in that ideal state. The group working on the architecture don't currently focus on compact for other platforms today. They are working on parity and performance of the new architecture vs the legacy architecture. While that group doesn't block their work on other platforms having implementations already, they do make sure, as best they can, that their approaches and boundaries could be implemented for the other platforms.

For the group working on the core components, they actively design their APIs with the desktop group, although the implementations start on mobile because the other platforms are downstream forks (like microsoft's GitHub repos)

For the meta, we could clearly still improve the workflow here.

@NickGerleman
Copy link
Contributor

Fix for the test is in facebook/react-native#31629

For now you can feel free to skip the test if we will integrate this soon, and everything else is working.

@rectified95 rectified95 added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 2, 2021
@ghost
Copy link

ghost commented Jun 2, 2021

Hello @rectified95!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@rectified95 rectified95 added AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) and removed AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) labels Jun 2, 2021
@ghost ghost merged commit c20df69 into microsoft:main Jun 3, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove fork of MapBufferBuilder.cpp

5 participants