Skip to content

Conversation

nabil-k
Copy link
Contributor

@nabil-k nabil-k commented Jun 30, 2022

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

This PR is based on the changes from #1511

Platform Status

  • Win32: Good, no known issues at the moment.
  • Windows: Good, no known issues at the moment.
    • It actually runs better than it's RN 0.64 counterpart. On RN 0.64 when switching pages you would get a render error that says Cannot redefine non-configurable property. However, after updating everything to RN 0.66, I haven't gotten that error at all.
  • MacOs: Good, no known issues at the moment.
  • iOS: Good, no known issues at the moment.
  • Android: Good, no known issues at the moment.

Description of Changes

These are just changes I've made to get further through the build process. This is just to help everyone be aware of some changes that have occurred between RN 0.64 and RN 0.66.

General

packages/utils/adapters/src/adapters
For RN 0.66 Typescript, two new props on TextProps have been added called onPressIn and onPressOut.

Windows

packages/theming/theming-utils/src/getCurrentAppearance.windows.ts
Removed !AppTheme.isAvailable, causes a type error.

Win32

packages/theming/win32-theme/src/NativeModule/getThemingModule.native.ts
Removed declaring TurboModuleRegistry in a react native module.

Verification

(how the change was tested, including both manual and automated tests)

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@nabil-k nabil-k requested a review from a team as a code owner June 30, 2022 02:00
@samuelfreiberg
Copy link
Contributor

"This is not meant to be merged as it currently stands." I think we should make this a Draft PR if it's not meant to be merged and there's a lot of work left (just so the intent is clear).

@nabil-k nabil-k marked this pull request as draft June 30, 2022 04:13
@Saadnajmi
Copy link
Collaborator

If you're feeling fancy, you could update it so that every package.json has an rnx-kit config and future version upgrades are a simple one-line rnx-dep-check --write. Totally not required though :)

@Saadnajmi Saadnajmi mentioned this pull request Jun 30, 2022
10 tasks
@nabil-k
Copy link
Contributor Author

nabil-k commented Jul 12, 2022

@rurikoaraki Yeah I think we should, I tested the onPressIn/Out prop and it works on all platform. I've added it, thanks!

@Saadnajmi
Copy link
Collaborator

Saadnajmi commented Jul 12, 2022

Minor note:
I think the test apps still specify "react-test-renderer": "17.0.1". That should probably be "17.0.2"

@nabil-k nabil-k marked this pull request as ready for review July 12, 2022 22:03
@nabil-k nabil-k marked this pull request as draft July 12, 2022 22:30
@nabil-k nabil-k marked this pull request as ready for review July 12, 2022 23:12
@nabil-k
Copy link
Contributor Author

nabil-k commented Jul 12, 2022

This PR causes this issue where Shadow is unable to pass the Jest tests due to an error "Invariant Violation: __fbBatchedBridgeConfig is not set, cannot invoke native modules"

@nabil-k nabil-k marked this pull request as draft July 13, 2022 16:28
@nabil-k nabil-k marked this pull request as draft July 13, 2022 16:28
@nabil-k nabil-k marked this pull request as ready for review July 13, 2022 20:34
@Dewsk
Copy link
Collaborator

Dewsk commented Jul 13, 2022

Nabil - great job here! We're really close to being ready to merge and this was a large cross-platform ask. I think we have some cool stories to tell about it and I've been really impressed with how diligently you've worked through all the tricky platform unique issues.

Copy link
Collaborator

@Dewsk Dewsk left a comment

Choose a reason for hiding this comment

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

We are good to merge this!

@rurikoaraki
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants