8000 Fixed useFetch issue by agagancarczyk · Pull Request #33999 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Fixed useFetch issue - #33999

Closed
agagancarczyk wants to merge 1 commit into
keycloak:mainfrom
agagancarczyk:issue-33968
Closed

Fixed useFetch issue#33999
agagancarczyk wants to merge 1 commit into
keycloak:mainfrom
agagancarczyk:issue-33968

Conversation

@agagancarczyk
@agagancarczyk agagancarczyk commented Oct 16, 2024 •
edited
Loading
Copy link
Copy Markdown
Contributor

Closes #33968

Fixes regression in #32974

@agagancarczyk
Fixed useFetch issue
6410369 
Signed-off-by: Agnieszka Gancarczyk <agagancarczyk@gmail.com>
@agagancarczyk
agagancarczyk requested a review from a team as a code owner October 16, 2024 12:59
@jonkoops
jonkoops commented Oct 16, 2024
Copy link
Copy Markdown
Contributor

LGTM, @agagancarczyk did you make sure to test that #32847 is not accidentally re-introduced? Also, what was the exact cause of the issue?

@agagancarczyk
agagancarczyk commented Oct 16, 2024
Copy link
Copy Markdown
Contributor Author

LGTM, @agagancarczyk did you make sure to test that #32847 is not accidentally re-introduced? Also, what was the exact cause of the issue?

Thanks @jonkoops Yes, I have tested it, but I've also asked @ssilvert to double-check, so let's give Stan a chance to try it out as well.

@jonkoops
jonkoops commented Oct 16, 2024
Copy link
Copy Markdown
Contributor

What was the exact nature of the bug? I am curious why this caused a loop.

@agagancarczyk
agagancarczyk commented Oct 16, 2024
Copy link
Copy Markdown
Contributor Author

What was the exact nature of the bug? I am curious why this caused a loop.

Oh yes, I think the repeated fetching was likely due to the showBoundary() call happening on every render or every dependency change, even when there was no error. That's my guess.

@keycloak-github-bot keycloak-github-bot Bot mentioned this pull request Oct 16, 2024
Closed
@agagancarczyk agagancarczyk mentioned this pull request Oct 16, 2024
Closed
@ssilvert
ssilvert commented Oct 16, 2024
Copy link
Copy Markdown
Contributor

LGTM, @agagancarczyk did you make sure to test that #32847 is not accidentally re-introduced? Also, what was the exact cause of the issue?

Thanks @jonkoops Yes, I have tested it, but I've also asked @ssilvert to double-check, so let's give Stan a chance to try it out as well.

I tried it out and unfortunately, this PR re-introduces #32847.

@ssilvert
ssilvert marked this pull request as draft October 16, 2024 17:51
@jonkoops jonkoops added the status/hold PR should not be merged. On hold for later. label Oct 17, 2024
@agagancarczyk
agagancarczyk commented Oct 18, 2024
Copy link
Copy Markdown
Contributor Author

Closing this PR so that it can be addressed in a different way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review
1 more reviewer
@jonkoops jonkoops jonkoops approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test status/hold PR should not be merged. On hold for later.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Not possible to close dialog boxes when clicking buttons or the close icon

3 participants

@agagancarczyk @jonkoops @ssilvert
0