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

Fix logout doesn't reset resource registration #7539

Merged
merged 12 commits into from
Apr 27, 2022

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Apr 13, 2022

Closes #7527 and #7586

  • Unregister resource definitions when resources change
  • Cover with unit tests

@fzaninotto fzaninotto added the RFR Ready For Review label Apr 13, 2022
Copy link
Contributor

@WiXSL WiXSL left a comment

Choose a reason for hiding this comment

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

I can still reproduce the bug described in #7527 locally, in the simple project. Just copy the index.tsx code of the proposed sandbox to the simple project's index.tsx

@fzaninotto
Copy link
Member Author

After thinking it through, I believe we should unregister resources when unmounting <Resource> rather than on logout. Because <Admin> accepts a function as child, the resources can vary according to external state.

@slax57
Copy link
Contributor

slax57 commented Apr 21, 2022

I added some tests to cover this.
However, I thought the last one would fail (justifying the need to unregister resources at unmount), but actually it doesn't.
I couldn't figure out why yet.
I think it would be better to understand this fully, and to be sure it will work in any cases before considering this issue fixed. If it's not the case, then I still need to implement François' suggestion + update my test case to reflect this.

@slax57 slax57 added WIP Work In Progress and removed RFR Ready For Review labels Apr 21, 2022
@fzaninotto fzaninotto added RFR Ready For Review and removed WIP Work In Progress labels Apr 26, 2022
@vercel vercel bot temporarily deployed to Preview – react-admin April 26, 2022 18:58 Inactive
@fzaninotto
Copy link
Member Author

I made sure the last test doesn't pass if we don't unregister the resource. The PR is ready for review.

@fzaninotto fzaninotto added this to the 4.0.2 milestone Apr 26, 2022
@fzaninotto
Copy link
Member Author

I can still reproduce the bug described in #7527 locally, in the simple project. Just copy the index.tsx code of the proposed sandbox to the simple project's index.tsx

I've tested it and the ready screen only shows up briefly. I've updated the docs and code to prevent it.

docs/Admin.md Outdated Show resolved Hide resolved
docs/Admin.md Outdated Show resolved Hide resolved
fzaninotto and others added 3 commits April 26, 2022 23:04
Co-authored-by: Aníbal Svarcas <WiXSL@users.noreply.github.com>
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

🔥

@djhi djhi merged commit c107fdd into master Apr 27, 2022
@djhi djhi deleted the fix-logout-keeps-resource-registration branch April 27, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v4: resources not reloading after login or auth change
4 participants