feat: add rest link to provide baseURL functionality

[WojciechMatuszewski]

This pull request is a draft implementation of rest link. This would allow the consumer to scope handlers to a given URL saving him some repetition.

Today was my first day of using this library and I could not find any reference on how to specify a baseURL for the rest handlers. Since there is similar functionality for the graphql ones I think it would be nice to have it also on the rest side.

Thank you for this great library!

• Closes Set base URL for matching relative paths in request #397

[kettanaito]

Hey, @WojciechMatuszewski! Thank you for preparing this feature!

It looks very good. I've left a few comments, could you please take a look? We are currently discussing whether link term has any value in the REST world. What is your opinion on this?

[WojciechMatuszewski]

Ok, so I see someone has actually created an issue that can be resolved with this PR. Given these circumstances, I will try to make the necessary changes during the weekend.

As for the link name, maybe we can go with root or base or something similar? I personally think that link as a name is fine.

[kettanaito]

@WojciechMatuszewski, let us know if you have any questions or would like to discuss things, we are here to help.

Let's keep the link for now then. Perhaps this is something we can align between all the request handlers and it would feel natural. Great job on this so far!

[WojciechMatuszewski]

Made some changes, the only problem I currently have is that I have no idea how to concatenate the base with path when base is a string and the path is a RegExp or the other way around. Maybe we should not support such a feature?

What's your opinion here @kettanaito ?

[kettanaito]

Thank you for all the awesome work you put into this, @WojciechMatuszewski! I'll look into the changes as soon as I can. We need to get this done, it's going to be a significant boilerplate reduction on the usage side of the library.

[kettanaito]

Hey, @WojciechMatuszewski. Thank you once more for these changes!

I've rebased your branch against the latest master and squashed the commits for a cleaner Git history. Then I refactored the rest.link creation in rest.ts, take a look to see how it turned out to be (we don't have to repeat type generics of the original createRestHandler anymore).

After that I've split the integration tests into separate suites, so it's easier to retain realistic names for rest.link handler in a single test suite.

There was a challenge to implement the mergeMasks, which you've already started with. In the end I think we should declare a limitation towards rest.link usage:

rest.link accepts a string that is an absolute URL of the resource.

Accepting a RegExp is both misleading and technically complex to support. Moreover, I don't think it's useful on practice.

Let me know what you think of these changes.

[kettanaito]

Creating a URL instance may fail if the base string contains path parameters (not a valid URL then). Fallback to joining the strings in that case.

[kettanaito]

Convert to URL and return its href to normalize two strings (double slashes, hashes, etc.).

[WojciechMatuszewski]

Hey, @WojciechMatuszewski. Thank you once more for these changes!

I've rebased your branch against the latest master and squashed the commits for a cleaner Git history. Then I refactored the rest.link creation in rest.ts, take a look to see how it turned out to be (we don't have to repeat type generics of the original createRestHandler anymore).

After that I've split the integration tests into separate suites, so it's easier to retain realistic names for rest.link handler in a single test suite.

There was a challenge to implement the mergeMasks, which you've already started with. In the end I think we should declare a limitation towards rest.link usage:

rest.link accepts a string that is an absolute URL of the resource.

Accepting a RegExp is both misleading and technically complex to support. Moreover, I don't think it's useful on practice.

Let me know what you think of these changes.

Great stuff! Thank you for polishing, now the changes are looking slick. I do not see anything that I could add here, but I wonder about the docs, should they be part of this PR as well?

[kettanaito]

We can add the docs once the changes are actually published to NPM and accessible to the users.

I've been thinking about this feature for some time and I have some doubts whether it grants enough benefit to outweigh its technical cost. Don't get me wrong, these changes are good, but I'm evaluating them from the API design perspective. Let's give it some time to think about it.

What troubles me is that you can achieve the same with this:

const github = (path) => {
  return `https://api.github.com/${path}`
}

rest.get(github('/users/:username'), resolver)

You pay the price of functional composition, but you get much more versatility. There's no request URL matching ambiguity on the library's side, the expectations are clear.

In comparison, when you create links like so:

const github = rest.link('https://api.github.com')

There are so many questions that arise:

• Can I use path parameters in the base URL?
• What if there are two path parameters with the same name in both base URL and the actual relative URL of the handler?
• How to deal with a request handler mask being a RegExp that contradicts the base URL? For example:

// Target all requests which URL doesn't start with "https"
github.get(/[^https]/, ...)

It also brings quite a lot of complexity to the request URL matching process in the library's internals, mainly due to all the possible combinations of the base URL value and its relative child paths.

[timdeschryver]

My 2 cents...

At first, I thought this feature would come in handy.
But I have to agree with @kettanaito , that this increases the API surface... and for what?
This might cause bugs/breaking changes in the future. Plus once this is added to MSW, it might not be sufficient for everyone's edge cases in the feature and this feature will grow over time.

Since it's simple to write a helper method in the user's codebase, I think it's not worth it to add it to MSW.
Also, since paths allow wildcards you can accomplish the same thing if you write your urls as */api/persons.

[WojciechMatuszewski]

@kettanaito very good arguments. I have no strong opinion here but as you pointed out there are a lot of things we would have to think about to make sure this feature is properly implemented.

I feel like we can just add a note in the docs about creating a simple helper function just like the one suggested and close this PR.

[marcosvega91]

I have read the thread yesterday and I was thinking about a solution. I agree with all of you that users can create their own function to make this works. I'm pretty sure that this is the right road to follow. I didn't think at the beginning about the all this side effect.

This feature could bring more problem than benefits.
IMO if there is a easy workaround for a feature it is not necessary to implement and leave the code cleaner.

[balavishnuvj]

I do agree with @kettanaito, Personally most of projects api point to same domain, and for tests I assume api domain is localhost.
But I do have a concern for tests pointing to different domain

folks doing this

let axiosInstance = axios.create({
    baseURL: apiUrl
});

They would have to compose url in every test. Which seems okay.

But the benefit of having such utility is very small.

[kettanaito]

@balavishnuvj, that's a good example. However, when you configure one tool you shouldn't expect another tool to somehow inherit that configuration. That's implicit programming and should it be discouraged. I'd recommend abstracting apiUrl into a constant and reusing it for both runtime and tests.

We've recently revamped the Request matching page that should drop some more light on how requests are matched, so that developers don't expect relative requests to be automagically matched against a base URL defined in the request client.

[balavishnuvj]

Yes, I wouldn't be expecting other libraries to inherit the config. But here I could since this library is focused in mocking there could be the way that I can override the default base(localhost) to something custom.

I would unserstand why it wouldn't work by default but would expect an API to do that.

const apiUrl= 'https://api.github.com';

// and usuage in regular app would be
let axiosInstance = axios.create({
    baseURL: apiUrl
});
axiosInstance.get('users/1234')
axiosInstance.get('users')

// and in msw
const github = (path) => {
  return `https://api.github.com/${path}`
}
rest.get(github('/users/:username'), resolver)
rest.get(github('/users'), resolver)

[kettanaito]

Even if we circumvent my points from above by forcing the base URL to always be a string representation of an absolute URL, it still creates a lot of mess during the request matching internally. The combinations like string + RegExp are utterly confusing, and the only effective way to solve this is to add something like multi-step request matching, which is a huge complication of the otherwise plain logic.

This feature could work by limiting both base URL and its child URL to always be strings. That substantially reduces the flexibility request handlers give you, such as targeting multiple requests based on a path or RegExp.

[kettanaito]

Unfortunately, I have to close this. See the reasoning behind why this feature costs more than it brings.

It's officially recommended to create a custom high-order function if you wish to reuse the same base path for multiple handlers. Like so:

const github = (path) => {
  return new URL(path, 'https://github.com').toString()
}

rest.get(github('/repos/:owner/:repo'), resolver)