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.

Sign up for GitHub

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

Jump to bottom

chore(react): use updater callback function #6424

Merged
merged 2 commits into from
Sep 22, 2022
Merged

chore(react): use updater callback function #6424

merged 2 commits into from
Sep 22, 2022

Conversation

virtuoushub
Copy link
Collaborator

@virtuoushub virtuoushub commented Sep 18, 2022 •
edited by Tobbe

callback takes the previous state as the first argument.

setState() does not always immediately update the component. It may batch or defer the update until later. This makes reading this.state right after calling setState() a potential pitfall. Instead, use componentDidUpdate or a setState callback (setState(updater, callback)), either of which are guaranteed to fire after the update has been applied. If you need to set the state based on the previous state, read about the updater argument below.

see:

Closes #5627

@Tobbe Tobbe added the release:fix This PR is a fix label Sep 21, 2022
@Tobbe Tobbe enabled auto-merge (squash) September 21, 2022 19:06
@virtuoushub
chore(react): use updater callback function
f327737 
callback takes the previous state as the first argument.

> setState() does not always immediately update the component. It may batch or defer the update until later. This makes reading this.state right after calling setState() a potential pitfall. Instead, use componentDidUpdate or a setState callback (setState(updater, callback)), either of which are guaranteed to fire after the update has been applied. If you need to set the state based on the previous state, read about the updater argument below.

see:
- https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-access-state-in-setstate.md
- https://reactjs.org/docs/react-component.html#setstate
@virtuoushub virtuoushub force-pushed the chore/avoid-accessing-this.state-within-setState branch from e47bd1c to f327737 Compare September 21, 2022 21:45
@virtuoushub
Copy link
Collaborator Author

Seeing some odd test failures. Seems like maybe a flakey test?

...
  ● can display a loading screen with a hook

    Rendered fewer hooks than expected. This may be caused by an accidental early return statement.

      90 |       if (loadingTimeMs) {
      91 |         setTimeout(() => {
    > 92 |           setAuthLoading(false)
         |           ^
      93 |           setAuthIsAuthenticated(true)
      94 |         }, loadingTimeMs)
      95 |       }
...

https://github.com/redwoodjs/redwood/actions/runs/3101321311/jobs/5022567567#step:8:1475

@virtuoushub virtuoushub force-pushed the chore/avoid-accessing-this.state-within-setState branch from c6eaf31 to 37f26b2 Compare September 21, 2022 23:35
@virtuoushub
Copy link
Collaborator Author

Still seeing some odd test results: https://github.com/redwoodjs/redwood/actions/runs/3101811253/jobs/5023535465#step:8:1287

...
● redirect replacing route

    Unable to find an element with the text: Home Page. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

    Ignored nodes: comments, script, style
    <body>
      <div>
        <h1>
          List Page
        </h1>
      </div>
    </body>

      1397 |   act(() => back())
      1398 |   // without options.replace = true in Redirect, back would go to List Page
    > 1399 |   await waitFor(() => screen.getByText('Home Page'))
           |         ^
      1400 | })
      1401 |
      1402 | describe('trailing slashes', () => {

      at waitForWrapper (../../node_modules/@testing-library/dom/dist/wait-for.js:187:27)
      at Object.<anonymous> (src/__tests__/router.test.tsx:1399:9)
...

not quite sure what is going on, but this one seems like an actual regression.

@Tobbe Tobbe merged commit 803e116 into redwoodjs:main Sep 22, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Sep 22, 2022
@Tobbe
Copy link
Member

Tobbe commented Sep 22, 2022

not quite sure what is going on, but this one seems like an actual regression.

All tests passed now, so I went ahead and merged. I've not been all to happy about the state of our regression tests lately though, randomly failing more often than not 🙁 So I think it's probably worth it soon to see if we can do something about that

@virtuoushub virtuoushub deleted the chore/avoid-accessing-this.state-within-setState branch September 22, 2022 17:36
@virtuoushub
Copy link
Collaborator Author

not quite sure what is going on, but this one seems like an actual regression.

All tests passed now, so I went ahead and merged. I've not been all to happy about the state of our regression tests lately though, randomly failing more often than not 🙁 So I think it's probably worth it soon to see if we can do something about that

+💯 - As a small related aside, I have found my work on #4992 is rife w/ what look to be test failures due to flakey / poorly written tests.

@jtoar jtoar modified the milestones: next-release, v3.1.0 Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtoar jtoar Awaiting requested review from jtoar

@Tobbe Tobbe Awaiting requested review from Tobbe

Assignees

@virtuoushub virtuoushub

Labels
release:fix This PR is a fix
Projects
Main
Status: Done
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

AuthProvider can cause "Warning: Can't perform a React state update on an unmounted component."
3 participants
@virtuoushub @Tobbe @jtoar