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

Add option to enable strict mode render #338

Closed
alexkrolick opened this issue Mar 31, 2019 · 21 comments · Fixed by #1241
Closed

Add option to enable strict mode render #338

alexkrolick opened this issue Mar 31, 2019 · 21 comments · Fixed by #1241
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alexkrolick
Copy link
Collaborator

Why: to test that components work in concurrent React

@kentcdodds
Copy link
Member

I understand the motivation, but couldn't this be accomplished using the wrapper option?

@alexkrolick
Copy link
Collaborator Author

alexkrolick commented Apr 1, 2019

Hmm, yes. I thought you have to change the ReactDOM.render() call

https://reactjs.org/docs/strict-mode.html

@kentcdodds
Copy link
Member

We would if we wanted to run concurrent mode. Maybe we will when that's stable, but I kinda don't think we will want/need to. We'll see.

lucbpz pushed a commit to lucbpz/react-testing-library that referenced this issue Jul 26, 2020
@penx
Copy link

penx commented May 18, 2022

We would if we wanted to run concurrent mode. Maybe we will when that's stable, but I kinda don't think we will want/need to. We'll see.

@kentcdodds sooo.... how about now? 😀

Coming to this issue as I have a hook that is behaving incorrectly in strict mode during development and I'd like to write a failing test before writing the fix.

I'll look in to the wrapper option.

@penx
Copy link

penx commented May 18, 2022

I see I can add wrapper: React.StrictMode to options:

    renderHook(() => useMyHook(), {
      wrapper: React.StrictMode,
    });

Unfortunately I'm pretty sure this is running React in production mode, or at least it isn't double-invoking useMemo as per https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

I don't see a way to make Jest/react-testing-library run React in development mode, perhaps there is one and I've missed it.

Obviously I want my hook to work in development mode (and I'm pretty sure there's an underlying issue here I'd need to solve anyway) and I'd really like to write a test to show this first.

Any thoughts?

@penx
Copy link

penx commented May 19, 2022

it isn't double-invoking useMemo as per https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

I think this assumption was due to React 17 silencing the console during double invoking.

Jest runs React with NODE_ENV=test by default, which enables development flags, so...

  • upgrading to React 18
  • using the new renderHook from react-testing-library (rather than react-hooks-testing-library)
  • using wrapper: React.StrictMode

...seems to be all I need to do to test my hook works when double invoking 😓

@smmoosavi
Copy link

I understand the motivation, but couldn't this be accomplished using the wrapper option?

Yes, The wrapper option can, but sometimes we need it for other components (e.g. DataProvider); strict options make it easier.

also, I think it could be good that we have a strict option enabled by default

@nickmccurdy
Copy link
Member

nickmccurdy commented Jan 4, 2023

Is there a reason not to enable this by default? Considering Create React App and Vite already default to enabling it.

Reopening this to make the discussion more visibile.

@nickmccurdy nickmccurdy reopened this Jan 4, 2023
@nickmccurdy nickmccurdy added the needs discussion Whether changes are needed is still undecided and additional discussion is needed. label Jan 4, 2023
@eps1lon
Copy link
Member

eps1lon commented Jan 4, 2023

Not a fan of altering the test input. Especially since StrictMode will break perfectly fine test (e.g. asserting the number of console calls).

The general advise has always been to use an internal wrapper instead. That is a good place to add StrictMode (we did that in MUI).

@paul-sachs
Copy link

paul-sachs commented Mar 29, 2023

Trying to write some tests for a hook that misbehaves in strict mode and can't figure out how to use wrapper along with calls to rerender.

const { rerender } = renderHook((dep) => useSomeHook(dep), {
    initialProps: 1,
    wrapper: StrictMode
});
rerender(2);

wrapper seems to expect a series of props here (children) but my hook only takes an integer. It's unclear how to handle this from the docs.

Edit
So you can get around this by just passing in something like the following:

const { rerender } = renderHook(({ value }) => useSomeHook(value), {
  initialProps: { value: 1, children: null },
  wrapper: StrictMode,
});

rerender({ value: 2, children: null });

But that does seem fairly verbose.

@eps1lon
Copy link
Member

eps1lon commented Mar 29, 2023

initialProps is for the hook not the wrapper component.

@paul-sachs

This comment was marked as off-topic.

@eps1lon

This comment was marked as off-topic.

@paul-sachs

This comment was marked as off-topic.

@eps1lon
Copy link
Member

eps1lon commented Aug 8, 2023

Yeah let's do this. I think it's currently a bit too verbose if you want StrictMode and a custom wrapper. Especially if we want to encourage testing in StrictMode to flush out issues when concurrently rendering early.

Not sure if we want to go with strict though in case we want to use that for something testing-library related. strictMode has a more obvious connection with React.StrictMode. Does anybody have a preference here?

@eps1lon eps1lon added enhancement New feature or request good first issue Good for newcomers and removed needs discussion Whether changes are needed is still undecided and additional discussion is needed. labels Aug 8, 2023
@nickmccurdy
Copy link
Member

nickmccurdy commented Aug 8, 2023

We could use the same name as Next's boolean option: reactStrictMode

@eps1lon
Copy link
Member

eps1lon commented Aug 8, 2023

The react prefix feels redundant here but using something familiar is a good enough argument for me to lean towards reactStrictMode.

And this option could also take its default from the configuration. That way verbosity isn't too much of an issue since you can likely opt-into StrictMode and then slowly fix the reactStrictMode: false instances.

@nickmccurdy
Copy link
Member

nickmccurdy commented Aug 8, 2023

The react prefix feels redundant here but using something familiar is a good enough argument for me to lean towards reactStrictMode.

My primary concern with strictMode is that it could be mistaken for JavaScript's strict mode, which potentially affects CJS modules in Node environments. Additionally I'm considering bundling APIs for RSC support, in which case toggling JS features like strict mode would be in the domain of RTL (or a supplemental package, I'll propose a specific architecture when I'm ready).

And this option could also take its default from the configuration. That way verbosity isn't too much of an issue since you can likely opt-into StrictMode and then slowly fix the reactStrictMode: false instances.

Are you referring to some existing config like next.config.js?

@eps1lon
Copy link
Member

eps1lon commented Aug 9, 2023

Are you referring to some existing config like next.config.js?

The one we have in @testing-library/react under configure(). For now we would default to reactStrictMode: false though and then discuss later if we want to flip it in a new major release.

@yinm
Copy link
Contributor

yinm commented Aug 30, 2023

Can I try this issue?

@eps1lon
Copy link
Member

eps1lon commented Jan 30, 2024

Fixed in #1241

@eps1lon eps1lon closed this as completed Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants