Skip to content
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

RNW 0.66.1: A warning is emitted if AppTheme is used at runtime and jest unittests fail. #8887

Closed
nichamp opened this issue Oct 21, 2021 · 4 comments · Fixed by #8901
Closed

Comments

@nichamp
Copy link

nichamp commented Oct 21, 2021

Problem Description

Issue 1
If there is a reference to AppTheme, a yellow-box warning for 'new NativeEventEmitter()' was called with a non-null argument without the required 'removeListeners' method appears within the app and in console logs. It seems that RNW's implementation of NativeModules.RTCAppTheme does not meet the expectations of NativeEventEmitter. I speculate this is related to the event listener changes in RN and/or with #8277

Issue 2
If one makes references to AppTheme in jest unittests with the default react native jest presets then it assumes assumes the platform is ios and at runtime it will fail an assertion with 'new NativeEventEmitter() requires a non-null argument.'. If one changes the platform to android it will emit warnings later instead, but either way AppTheme will not function correctly unless NativeModules.RTCAppTheme is mocked/shimmed before AppTheme is referenced. Perhaps this is now the expected behavior, but it has caused problems migrating from 0.64 to 0.66 for existing tests.

Steps To Reproduce

Issue 1

  1. Create an RNW 0.66.1 app using the CLI
  2. Add a references to App.tsx such as AppTheme.isHighContrast
  3. Run the application (e.g. yarn start with metro will repor) and observe the yellow box warning

Issue 2

  1. Create a jest unittest that references AppTheme and use the jest react-native default which assumes the platform is ios.
  2. Observe the test failing with 'new NativeEventEmitter() requires a non-null argument.'. If one changes the platform to android it will emit warnings later instead.

Expected Results

Issue 1
No warnings should be omitted. RNW's built-in native module should follow the expectations of RN where possible.

Issue 2
There should be a built-in mock for the native module so that AppTheme can be safely referenced in jest unittests without the developer needing hacks like assigning to NativeModules.RTCAppTheme directly.

CLI version

6.1.0

Environment

"dependencies": {
    "react": "17.0.2",
    "react-native": "0.66.1",
    "react-native-windows": "0.66.1"
  },
  "devDependencies": {
    "@babel/core": "^7.12.9",
    "@babel/runtime": "^7.12.5",
    "@react-native-community/eslint-config": "^2.0.0",
    "@types/jest": "^26.0.23",
    "@types/react-native": "^0.65.0",
    "@types/react-test-renderer": "^17.0.1",
    "babel-jest": "^26.6.3",
    "eslint": "^7.14.0",
    "jest": "^26.6.3",
    "metro-react-native-babel-preset": "^0.66.2",
    "react-test-renderer": "17.0.2",
    "typescript": "^3.8.3",
    "metro-config": "^0.66.2"
  },

Target Platform Version

10.0.19041

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2019

Build Configuration

Debug

Snack, code example, screenshot, or link to a repository

No response

@nichamp nichamp added the bug label Oct 21, 2021
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Oct 21, 2021
@asklar asklar removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Oct 22, 2021
@asklar
Copy link
Member

asklar commented Oct 22, 2021

spoke to @NickGerleman, he'll take a look.

The solution here is that we should mock it in the jest preset, like RN does for all the built-in modules

@asklar asklar added this to the 0.67 milestone Oct 22, 2021
@asklar asklar added the must-have p1 label Oct 22, 2021
@NickGerleman
Copy link
Collaborator

@nichamp I noticed you mentioned running against Android/iOS platform. Just wanted to give an FYI that we have some tools to make Jest do the right thing for out of tree platforms, at https://github.com/microsoft/rnx-kit/tree/main/packages/jest-preset. This will cause jest to include the right files per-platform, and allows you to specify platform as a command line option.

Need to look at #1. Unsure what would be causing the warning until I debug it or see if there were any upstream changes.

Re #2, as Alex quoted above, I think the right architecture here is for RNW to set up its mocks in a Jest preset we export, like is done for react-native itself. This wasn't something we documented though, so I think it would be righteous for me to restore behavior of being able to use AppTheme without an RNW Jest preset in the meantime, so we can be intentional about changing any sort of requirements.

@nichamp
Copy link
Author

nichamp commented Oct 22, 2021

For (1) there are actually two warnings actually (I had only noticed the first at the time I created the issue):

image

image

For (2), I am currently using a workaround where my tests inject a mock of NativeModules.RTCAppTheme so they are unblocked in the interim. It definitely would be helpful if the number of breaking changes between releases was minimized but I think authors' code and tests would be broken here by RN's change to how the subscriptions work too.

NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Oct 22, 2021
Fixes microsoft#8887

Fixes warnings emitted due to upstream NativeEventEmitter refactoring, along with a more specific typing for the API to add a listener.

Fix an uninetional  behavior change when AppTheme is imported in a jest environment.

Not manually validated yet due to some local issues.
NickGerleman added a commit that referenced this issue Oct 24, 2021
* Fix AppTheme Regressions

Fixes #8887

Fixes warnings emitted due to upstream NativeEventEmitter refactoring, along with a more specific typing for the API to add a listener.

Fix an uninetional  behavior change when AppTheme is imported in a jest environment.

Not manually validated yet due to some local issues.

* Change files
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Oct 24, 2021
* Fix AppTheme Regressions

Fixes microsoft#8887

Fixes warnings emitted due to upstream NativeEventEmitter refactoring, along with a more specific typing for the API to add a listener.

Fix an uninetional  behavior change when AppTheme is imported in a jest environment.

Not manually validated yet due to some local issues.

* Change files
NickGerleman added a commit that referenced this issue Oct 24, 2021
* Fix AppTheme Regressions

Fixes #8887

Fixes warnings emitted due to upstream NativeEventEmitter refactoring, along with a more specific typing for the API to add a listener.

Fix an uninetional  behavior change when AppTheme is imported in a jest environment.

Not manually validated yet due to some local issues.

* Change files
@NickGerleman
Copy link
Collaborator

Resolved in 0.66.2 releasing Monday morning.

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

Successfully merging a pull request may close this issue.

3 participants