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

Add convenience methods to request() functions for HTTP verbs #765

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

matthew-white
Copy link
Member

This PR adds convenience methods for HTTP verbs to the request() function returned by useRequest(), as discussed here. For example, the following two calls will send the same request:

const { request } = useRequest();
request({
  method: 'POST',
  url: '/v1/projects',
  data: { name: 'My Project' }
});
request.post('/v1/projects', { name: 'My Project' });

The parameters of the convenience methods are the same as they were in the request mixin.

I want to make the request() function returned by useRequest() and the request() method of requestData resources as similar as possible (see #675). With that in mind, I've also added the convenience methods to the request() method. The request() method usually sends GET requests, but there are several cases where it sends a non-GET request.

What has been done to verify that this works as intended?

New tests.

Why is this the best possible solution? Were any other approaches considered?

In this PR, useRequest() and requestData call a shared function named withHttpMethods(), which adds convenience methods to any request() function/method. The main question I had was how to implement withHttpMethods(). I wanted to avoid eagerly creating five additional functions for every component that calls useRequest(), because most components will use at most one convenience method (and many components won't use a convenience method at all). I tried two different working approaches, which are both in the commit history.

One approach I considered was to subclass Function. The convenience methods would go on the subclass's prototype. However, after briefly investigating this option, it sounded like extending Function involved complications.

The next approach I considered was to wrap the request() function in a proxy. The proxy would behave the same as the request() function except when getting one of the convenience methods. In that case, the proxy would create the convenience method on the fly. I got this approach to work, and you can see it in the commit history.

I then realized that if the convenience methods are properties of the request() function, they can access the request() function via this. That means that we don't need to create different convenience methods for each request() function: all request() functions can use the same convenience methods. So the approach I ended up taking was to define the convenience methods once, then have withHttpMethods() assign them on the request() function.

this ended up coming up in another issue I encountered, which is that calling a convenience method results in a different this compared to calling request() directly. That isn't an issue for useRequest(), but it comes up for requestData, because the request() method references this. For example, if you call someResource.request(), then this is someResource. But if you call someResource.request.post(), then this is someResource.request. I was able to resolve this by binding someResource.request to someResource.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This change is not user-facing.

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

No changes needed.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

// bind it to the proxy. For example, if resource.request.post() is
// called, we need `this` to be the proxy in post().
if (prop === 'request')
return withHttpMethods(resource.request.bind(proxy));
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure at first where to do this binding. Initially, I put it in the constructor of the Resource class above. However, that binds the request() method to the Resource instance. Instead, we want to bind it to the resource proxy. The resource proxy is what is returned from this file: the file only uses the Resource instance internally.

Base automatically changed from use-request to master March 31, 2023 19:46
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

looks good.

src/util/request.js Show resolved Hide resolved
@matthew-white matthew-white merged commit a6b3611 into master Apr 14, 2023
@matthew-white matthew-white deleted the with-http-methods branch April 14, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants