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

Make temporary NoStrictPassiveEffects option work with useModernStrictMode #27105

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

tyao1
Copy link
Contributor

@tyao1 tyao1 commented Jul 12, 2023

Summary

Since we are enabling useModernStrictMode flag internally, to make sure the internal testing of half StrictMode doesn't suddenly break, this PR makes sure it also works with useModernStrictMode true.

Test plan:

Manually set useModernStrictMode to true.
yarn test ReactOffscreenStrictMode-test -r=www-modern --env=development --variant=true
yarn test ReactStrictMode-test.internal -r=www-modern --env=development --variant=true

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 12, 2023
doubleInvokeEffectsOnFiber(
root,
fiber,
(fiber.mode & NoStrictPassiveEffectsMode) === NoMode,
Copy link
Member

Choose a reason for hiding this comment

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

should the mode be passed down below on L3597 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I checked the test case where a Suspense boundary is used

it('should include legacy + strict effects mode, but not strict passive effect with disableStrictPassiveEffect in Suspense', async () => {
await act(() => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
root.render(
<React.StrictMode DO_NOT_USE_disableStrictPassiveEffect={true}>
<React.Suspense>
<Component label="A" />
</React.Suspense>
</React.StrictMode>,
);
});
expect(log).toEqual([
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useLayoutEffect mount',
]);

and the codepath on L3597 doesn't hit. So no, we don't need this flag for components under a real Offscreen component.

@tyao1 tyao1 merged commit d445cee into main Jul 18, 2023
@tyao1 tyao1 deleted the ty/less-strict-mode branch October 12, 2023 17:39
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…rictMode` (facebook#27105)

## Summary
Since we are enabling `useModernStrictMode` flag internally, to make
sure the internal testing of half StrictMode doesn't suddenly break,
this PR makes sure it also works with `useModernStrictMode` true.


## Test plan:
Manually set `useModernStrictMode` to true.
`yarn test ReactOffscreenStrictMode-test -r=www-modern --env=development
--variant=true`
`yarn test ReactStrictMode-test.internal -r=www-modern --env=development
--variant=true`
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…rictMode` (#27105)

## Summary
Since we are enabling `useModernStrictMode` flag internally, to make
sure the internal testing of half StrictMode doesn't suddenly break,
this PR makes sure it also works with `useModernStrictMode` true.

## Test plan:
Manually set `useModernStrictMode` to true.
`yarn test ReactOffscreenStrictMode-test -r=www-modern --env=development
--variant=true`
`yarn test ReactStrictMode-test.internal -r=www-modern --env=development
--variant=true`

DiffTrain build for commit d445cee.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants