Add responseInterceptors feature to ES UI Shared request module. #113671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjcenizal
added
enhancement
|
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
cjcenizal
commented
Oct 1, 2021
| it('applies errorInterceptors to errors', async () => { | ||
| const { setupErrorRequest, completeRequest, hookResult, getErrorResponse } = helpers; | ||
| const errorInterceptors = [ | ||
| (error: any) => ['Error is:', error.statusText], |
There was a problem hiding this comment.
The functionality described in the PR description can be accomplished by creating side effects within an error interceptor, e.g. a call to setState.
cjcenizal
force-pushed
the
feature/request-error-interceptors
branch
from
4f8bfe2 to
dd036e2
Compare
8 tasks
cjcenizal
force-pushed
the
feature/request-error-interceptors
branch
from
dd036e2 to
9947d65
Compare
cjcenizal
changed the title
…ocus interceptors on executing side effects, not mutating responses.
cjcenizal
force-pushed
the
feature/request-error-interceptors
branch
from
c44b2c9 to
b74c685
Compare
|
Thanks for working on this CJ! I haven't got into details in the code but I notice that you suggest to provide to each request the interceptor. I would like to see concrete examples of where this would be used (or maybe understand better the problem we are trying to solve) as it seems a bit verbose for me from a consumer point of view. As I understand them, request interceptors are an app wide thing declared in a single place, probably inside a React Context layer. This then allows us to pass down any errors down the tree and have dedicated UIs that react to those errors. If I am wrong and we are simply trying to solve a "enhance the response" problem, then your solution could work or we could get away by simply having app handlers wrapping calls to If we need an "app wide" layer to pass down the tree possible errors maybe we could follow the pattern that the form lib uses with the schema and the validations declared in a single place? This is how it would look like. (very quick and dirty example 😊 ) // Declare the interceptors in a single place
const requestInterceptors = {
// the key is the request path
'/api/index_management/load_indices': {
method: 'POST',
handlers: [
(response) => {
if (response.somethingIsNotRight) {
return {
message: 'Houston we got a problem',
code: 'ERR_CODE',
}
}
},
... // another handler
]
}
};
// Render the app
<RequestInterceptorProvider interceptors={requestInterceptors}>
...
</RequestInterceptorProvider/>
// Inside the context provider
const RequestInterceptorProvider = ({ interceptors }) => {
const [errors, setErrors] = useState([]);
// Enhance the sendRequest with the interceptors
const sendRequestWithInterceptors = useCallback(() => {
const rawResponse = await sendRequest(httpClient, options);
if (interceptors[options.path] && interceptors[options.path].method === options.method) {
const requestErrors = interceptors[options.path].handlers
.map((handler) => handler(rawResponse))
.filter(Boolean);
setErrors(requestErrors);
if (requestErrors.length) {
return {
error: requestErrors[0], // return the first one to the consumer
data: null
};
}
}
return rawResponse;
}, []);
return (
/* The errors are available to any children UI */
<Interceptor.Provider value={{ errors, sendRequest: sendRequestWithInterceptors }}>
{children}
</Interceptor.Provider>
)
};
// Make a normal request from anywhere in the app
const MyComponentInTheTree = () => {
const { useRequest } = useAppDependencies(); // // Access the "useRequest()" through context
// Make a normal request as usual ...
const { data } = useRequest(...);
};Let see from concrete examples of the problem we try to solve if my solution makes sense. 👍 |
yuliacech
reviewed
Oct 4, 2021
There was a problem hiding this comment.
Hi @cjcenizal, great work with this addition to the useRequest hook. I found the logic of the changes straight forward enough to understand the intent, so those LGTM!
My only concern was with the interceptors relying on mutating the response or error object. I think mutating objects can often lead to some hard-to-trace bugs.
I then checked how you are using interceptors in #112907 and I saw that their purpose is not to update the response or error object, but more to perform some side effects on the component. Is my understanding correct here?
So I think that the approach is working well, but maybe the naming could be more specific? Maybe also some comments could help to explain the intent. I'm also wondering if mutating response/error objects should even be impossible when using interceptors, i.e. always create a new object when updating anything.
|
Thanks for the review @yuliacech!
To be clear, this was my original approach, but in my second commit I reversed this so that the interceptors no longer mutate anything. Would it help if I added some tests to assert that responses and errors are unchanged by the interceptors?
Yes, that's correct. The intention is to perform side effects based on the response or error.
Sure happy to make these changes. Is |
|
BTW I just fixed the places in the linked PR that consume this feature. See https://github.com/elastic/kibana/pull/112907/files#diff-905e5959a1aabc2473bcfb27ebb8cee8b81d9bd0ffe441b651f5848a2f7de6b8 |
|
Thank you for the clarification, @cjcenizal! I actually see the difference between commit 1 and commit 2 now and the change in the approach: mutating vs side effects. I think 'interceptor' is for me 'mutating' and passing the response further, so I'd suggest using 'responseHandler' and adding a comment about side effects here. |
|
I like it! Thanks @yuliacech. I implemented your suggestion. Could you take another look? |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/dashboard_elements/input_control_vis/input_control_range·ts.dashboard elements dashboard elements ciGroup10 input controls input control range should add filter with scripted fieldStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/management/_scripted_fields·js.management scripted fields creating and using Painless date scripted fields should see scripted field value in DiscoverStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/management/_scripted_fields·js.management scripted fields creating and using Painless date scripted fields should see scripted field value in DiscoverStandard OutStack TraceMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
|
I have some concerns with the current solution:
The solution I quickly put in my comment:
I'd like to discuss this further before going forward with this solution. Maybe over zoom? |
|
@sebelga Would you mind explaining your concerns in more detail?
It's true that in my use case, each request handles errors in the same way (except for the telemetry request). How do you see this being problematic?
What's the downside you're seeing?
🤔 This is correct but I'm not see this problem... can you elaborate? |
|
And just to be transparent, the problems I'm interested in are problems with correctness, robustness, quality, maintainability, readability, things like that. Some of the questions I'd ask are: Does the code work? Is it understandable? Can it be maintained? Can it be scaled? Does it increase risk of bugs? |
yuliacech
approved these changes
Oct 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will enable consuming plugins to intercept errors and successful responses that apply to the entire app, and execute side effects such as updating the UI accordingly.
For example, #112907 requires Upgrade Assistant to block off the UI if a
426error is returned from any API route. This change will enable theapi.tsfile to define error interceptors to handle this type of error and update the UI accordingly.Note that this branch's history captures the evolution of this solution:
For reviewers: Does this approach make sense? Does it introduce any drawbacks, or anything confusing to the interface of the
requestmodule?