-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(iOS): support bridgeless mode. #4681
Conversation
dc250b6
to
0197120
Compare
Fabric flattens the view hierarchy if the elements are not accessible.
Callstack's slider view has a property called 'slider' in new arch (RCTViewComponentView), which is the actual slider. See - https://github.com/callstack/react-native-slider/blob/main/package/ios/RNCSliderComponentView.mm#L26
JSI timers are not being synced on new arch (iOS) since we cannot swizzle them.
fea3349
to
915275c
Compare
98fb331
to
ec530a8
Compare
@@ -2,9 +2,9 @@ describe(":ios: Background-Foreground Transitions", () => { | |||
it("Backgrounding and foregrounding an app should wait for transition to finish", async () => { | |||
await device.launchApp({newInstance: true}); | |||
await device.sendToHome(); | |||
await expect(element(by.text("Background"))).toBeVisible(); | |||
await expect(element(by.text("Background"))).toExist(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asafkorem why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a new native component (replaced the previous one), and when the app is on background this element is really not expected to be visible (only to exist).
Note that only the Active
message should be visible because the app is visible. i.e.
await expect(element(by.text("Active"))).toBeVisible();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bug that the previous message was considered as "visible" while the app was in the background. It's because that "AnnoyingWindow" we used to have - no longer exists.
@@ -51,8 +53,10 @@ describe('Crash Handling', () => { | |||
|
|||
if (device.getPlatform() === 'android') { | |||
jestExpect(error.stack).toContain('\tat java.lang.Thread.run'); | |||
} else { | |||
} else if (!isRNNewArch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asafkorem I think we need to revisit and decide:
- The test is
@legacy
so I thinkiSRNNewArch
is always false (?) - Should we backlog a tech debt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, we can remove the isRNNewArch
. Detox sometimes consider the app as launched before the error is thrown, so we'll have an error on the next action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a technical debt or at least a discussion - whether it's a bug or feature. It's a non-deterministic behaviour because sometimes detox catches the error on-launch and sometimes it will be thrown in the next action (post-launch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,4 +1,4 @@ | |||
describe.skipIfNewArchOnIOS(":ios: Picker", () => { | |||
describe(":ios: Picker", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amaze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
@@ -9,7 +9,7 @@ describe('React-Native Animations', () => { | |||
let loopSwitch = element(by.id('UniqueId_AnimationsScreen_enableLoop')); | |||
await loopSwitch.tap(); | |||
if (device.getPlatform() === 'ios') { | |||
await expect(loopSwitch).toHaveValue('1'); | |||
await expect(loopSwitch).toHaveToggleValue('1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a warning whenever .toHaveValue()
is used over a UISwitch
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, not sure, we can talk about that. AFAIK we never did it with other components (CMIIW)
It's a specific component behaviour that was changed over RN versions.. 🤷🏼♂️
It was really the accessibility value of the original component before
@@ -22,7 +22,8 @@ describe('Network Synchronization', () => { | |||
await driver.shortRequest.expectReplied(); | |||
}); | |||
|
|||
it('Sync with long network requests - 3000ms', async () => { | |||
// todo(new-arch): test is failing | |||
it('@legacy Sync with long network requests - 3000ms', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asafkorem this looks serious, what's the gap here?
No description provided.