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

useIsomorphicLayoutEffect cannot pass test cases in non-web environment #1436

Closed
malash opened this issue Oct 29, 2019 · 6 comments
Closed

Comments

@malash
Copy link

malash commented Oct 29, 2019

Do you want to request a feature or report a bug?

Report a bug.

What is the current behavior?

const useIsomorphicLayoutEffect =
typeof window !== 'undefined' &&
typeof window.document !== 'undefined' &&
typeof window.document.createElement !== 'undefined'
? useLayoutEffect
: useEffect

const useIsomorphicLayoutEffect =
typeof window !== 'undefined' &&
typeof window.document !== 'undefined' &&
typeof window.document.createElement !== 'undefined'
? useLayoutEffect
: useEffect

These two files implement the useIsomorphicLayoutEffect hook to prevent the React warning by using useEffect instead of useLayoutEffect. But it could break on non-web environment, like React Native, and etc.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to a CodeSandbox (https://codesandbox.io/s/new) or RN Snack (https://snack.expo.io/) example below:

I tested by these steps:

  1. Clone the react-redux repo
  2. Modify useIsomorphicLayoutEffect to const useIsomorphicLayoutEffect = useEffect
  3. Run npx jest

image

What is the expected behavior?

  1. We should add some test cases for non-web environment. For example @testing-library/react-native provides the way to test react-native in jest.
  2. We should review the implement of useIsomorphicLayoutEffect and fix it if there did have some bugs.

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

react-redux@7.1.1

cc

@byunicorn

@markerikson
Copy link
Contributor

No one has yet filed actual bugs reporting this doesn't work in React Native. Do you have any actual test cases that show it fails in an RN app?

@malash
Copy link
Author

malash commented Oct 29, 2019

😂Bug happened here #1437

@malash
Copy link
Author

malash commented Oct 29, 2019

B.T.W. I think it worth to add unit tests for RN because users did use react-redux in RN , right ?

@markerikson
Copy link
Contributor

Yep, but I wasn't sure how to try to add RN unit tests at the time.

@malash
Copy link
Author

malash commented Oct 29, 2019

How about @testing-library/react-native ?

@markerikson
Copy link
Contributor

Fixed by #1444 .

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

No branches or pull requests

2 participants