Skip to content

Integrate of 3/22 Nightly Build#5028

Merged
60 commits merged into
microsoft:masterfrom
NickGerleman:nightly-integration
Jun 26, 2020
Merged

Integrate of 3/22 Nightly Build#5028
60 commits merged into
microsoft:masterfrom
NickGerleman:nightly-integration

Conversation

@NickGerleman
Copy link
Copy Markdown
Contributor

@NickGerleman NickGerleman commented May 27, 2020

Start integration of the first available nightly build before 0.63. This build has some interesting stuff like C++ version stamping, PlatformColor, import fixes.

Fixes #3990

Microsoft Reviewers: Open in CodeFlow

NickGerleman and others added 13 commits May 27, 2020 09:44
Fixes microsoft#4857

Teach the tooling about the pattern and make sure we can validate against `0.0.0-56cf99a96` (the earliest nightly build). We cannot do a lazy fetch against a shortned commit hash, so we need to get a bit "creative" here.

We make a couple other improvements here:

1. Change fetch depth to 1 to pull in less history. This reduces the scratch repo size from 174MB to 34MB when fetching just 0.62.2 (which is already much less than the 500MB for cloning entire remote)
2. Don't add remotes to the scratch git repo and instead fetch using the URL. We've seen transient errors where `simplegit` thinks we don't yet have a repo set up and adds a duplicate remote. This should hopefully fix that.
Comment thread vnext/src/overrides.json Outdated

'use strict';
import ReactNativeViewViewConfigAndroid from './ReactNativeViewViewConfigAndroid';
import {Platform} from 'react-native'; // from facebook:master d204541
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did these comments mean?

Copy link
Copy Markdown
Contributor Author

@NickGerleman NickGerleman Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 0.61 we needed to pull in some changes made to later builds around processing transforms for IIRC non iOS platforms? Added a comment to the change for where the diff was from.


_setNativePropsOnRef: ?({refreshing: boolean, ...}) => void;
_nativeRef: ?React.ElementRef<
| typeof PullToRefreshViewNativeComponent
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Windows implement this one? Or do you need to add another one for Windows to this file?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We implement PullToRefreshViewNativeComponent.


import AndroidCheckBoxNativeComponent, {
Commands as AndroidCheckBoxCommands,
} from './AndroidCheckBoxNativeComponent';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Windows component have the Android name?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we have the android name. -- That way we can mark in our tooling that the .windows.js should just be a copy of the .android.js version. This reduces merge cost in the future.

We'd probably try to push a change upstream to make rename CheckBox.android.js -> CheckBox.js, and change the native component name to be more platform agnostic but since its deprecated I'm not sure its worth the effort.

Comment thread vnext/.flowconfig
<PROJECT_ROOT>/Libraries/Components/Picker/RCTPickerNativeComponent.js
<PROJECT_ROOT>/Libraries/Components/RefreshControl/RefreshControl.js
<PROJECT_ROOT>/Libraries/Components/SafeAreaView/SafeAreaView.js
<PROJECT_ROOT>/Libraries/Components/ScrollView/ScrollView.js
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do things being removed from this file mean you no longer have override files for them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want the "source of truth" for platform overrides you can also look at the "overrides.json"!files scattered around the repo. The stuff in the flowconfig is just where we replace something that's shared JS.

Comment on lines 45 to 51
if (global.RN$Bridgeless) {
ImageViewNativeComponent = codegenNativeComponent<NativeProps>(
'RCTImage', // [Win32] Rename RCTImageView to RCTImage
);
ReactNativeViewConfigRegistry.register('RCTImage', () => {
// [Win32] Rename RCTImageView to RCTImage
return ImageViewViewConfig;
});
ImageViewNativeComponent = 'RCTImage'; // [Win32] Rename RCTImageView to RCTImage
} else {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks shouldn't matter much for you right now, this is for Venice, the post-fabric project.

ImageViewNativeComponent = 'RCTImage'; // [Win32] Rename RCTImageView to RCTImage
} else {
ImageViewNativeComponent = requireNativeComponent<NativeProps>(
'RCTImage', // [Win32] Rename RCTImageView to RCTImage
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. in the source I see RCTImageView: https://github.com/facebook/react-native/blob/master/Libraries/Image/ImageViewNativeComponent.js#L51

Should these comments say "Rename RCTImage to RCTImageView"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends if its documenting what the win32 diff is compared to master, or if its documenting a change we need to make in native. In this case it looks like its trying to document the diff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've been commenting what the change is instead of as a TODO.

@NickGerleman
Copy link
Copy Markdown
Contributor Author

ACoates changed the ImageViewNativeCompoonent name in devmain to support old + new so we should be able to remove JS forking entirely this PR.

@NickGerleman
Copy link
Copy Markdown
Contributor Author

NickGerleman commented Jun 13, 2020

Also need to remove the last of the NativeStatusBarManager patches, since we aren't supporting anything anymore where a require-time dependency of the NM would break anything and don't need NativeOrDynamicColor tweaks any more.

@NickGerleman NickGerleman changed the title [WIP] Begin integration of 3/22 Nightly Build Integrate of 3/22 Nightly Build Jun 25, 2020
@NickGerleman NickGerleman marked this pull request as ready for review June 25, 2020 19:41
@NickGerleman NickGerleman requested a review from a team as a code owner June 25, 2020 19:41
@NickGerleman
Copy link
Copy Markdown
Contributor Author

@acoates-ms I have a few more quick fixes to do, and some followups, but I think we should be able to merge today. Would rather get master on nightly sooner than later to avoid some of the assumptions that have been coming up around it.

@NickGerleman NickGerleman added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 26, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jun 26, 2020

Hello @NickGerleman!

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.

@ghost ghost merged commit 0727ffc into microsoft:master Jun 26, 2020
@elicwhite
Copy link
Copy Markdown

7drHiqr

Copy link
Copy Markdown

@elicwhite elicwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when do you plan on starting to integrate another nightly build? 😀

Should be a much smaller diff hopefully 🤞

// onChange event to JS when JS itself is updating the value of the checkbox
shadow->UnregisterChangedEvents();
viewToUpdate.as<xaml::Controls::CheckBox>().IsChecked(value);
shadow->RegisterChangedEvents();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a return in this if statement to keep the next IsChecked call from running again?

Comment on lines +20 to +23
export const PlatformColor = (...names: Array<string>): ColorValue => {
// We dont support fallback colors right now, so no point in sending more than the first color across the bridge
return {windowsbrush: names[0]};
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this neat how WIndows can define a custom behavior for this function that works throughout the stack? It seems like this is something that would benefit from some documentation in core for other platforms to know how to do it. Who would be the right person to ask to help with that?

@@ -0,0 +1,21 @@
/**
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this file? It looks like it is unchanged? Should it be inherited from core? Or is it being here, without a platform extension, doing exactly that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was removed from OSS, then added back later, but after this build. We need it in conjunction with the Appearance changes around avoiding sync module calls to allow web debugging.

@NickGerleman
Copy link
Copy Markdown
Contributor Author

So when do you plan on starting to integrate another nightly build? 😀

Should be a much smaller diff hopefully

This build gets us two weeks away from the base of 63. At that point where we reach the base of 0.63 we will need to wait for other payload we would want for RNW 0.63. In the meantime my plan is to spend some time working on automatic integration tooling and use those two weeks to experiment with it.

Current discussion has pointed towards branching 0.63 to preview in mid-August, where we can resume integration after. This takes us from about three months behind FB master back to 4.5 months, which isn't ideal. @acoates-ms and I were talking about wanting to do this a bit earlier, but there's also some pretty critical changes we'd like to get into the next RNW, so there's a tradeoff. Need to discuss this more with @chrisglein next week when he's back from vacation.

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

4 participants