-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] useFetcher: Replace react-redux-request with hooks and Context API #33392
Conversation
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few mostly general questions on this first pass.
💔 Build Failed |
fae8c82
to
df70310
Compare
💚 Build Succeeded |
💚 Build Succeeded |
💔 Build Failed |
This is a great hook. I especially like how it only refetches when there are changes in the options object. LGTM! |
dd920c6
to
fd1cb71
Compare
💚 Build Succeeded |
💔 Build Failed |
12b93ac
to
8ee4cad
Compare
💔 Build Failed |
💔 Build Failed |
83aa30a
to
233e6c8
Compare
💚 Build Succeeded |
233e6c8
to
5e0bb4a
Compare
💚 Build Succeeded |
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
Fix test Address feedback [APM] Add useFetcher to ServiceOverview, and add `react-testing-library` Fix tests Make spec descriptions more clear [APM] Add useFetcher to serviceDetailsView Fix tests Add redirect Fix snapshot Fix redirect to correct bucket Convert TransactionDetails [APM] Fix Datepicker double loading and move date parsing to urlParams Convert more to hooks Revert "[APM] Fix Datepicker double loading and move date parsing to urlParams" This reverts commit ed41e17d57daf0f6053a6e7bd901a500d2c64f92.
556e1b3
to
5612dc7
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thing I can see is maybe a useFetcher test on the error scenario, because that would have probably caught the bug when the function was being called just outside of the try/catch block.
But everything here LGTM.
It totally would have! Will add a test for that case now, and then merge. |
💚 Build Succeeded |
💚 Build Succeeded |
…API (elastic#33392) # Conflicts: # x-pack/package.json # yarn.lock
First steps towards replacing react-redux-request with a custom hook. This PR just replaces rrr for the error overview and error details page.
Why Context API and hooks instead of Redux and react-redux-request (RRR: custom render prop component for data fetching)?
useFetch
hook, the state is local by default by default but can easily be lifted up with the context API.Tip: Setting the diff settings to "Hide whitespace changes" will make this a fair bit easier to review.