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

Some use-cases may require allowing an empty URL #4407

Open
felixh10r opened this issue Jan 20, 2022 · 22 comments
Open

Some use-cases may require allowing an empty URL #4407

felixh10r opened this issue Jan 20, 2022 · 22 comments

Comments

@felixh10r
Copy link

felixh10r commented Jan 20, 2022

Is your feature request related to a problem? Please describe.

I'd like to give some feedback for the axios 0.25 release. #3791 introduces a fast failure when an empty string is provided as a URL to the request builder. However, my own and at least one other project are intentionally using empty strings as URLs and the 0.25 release breaks working code. It is listed as a breaking change in the changelog, but the behavior may be unpractical for some use-cases: Let's imagine a service abstraction over a RESTful resource; the baseURL could be something like http://example.com/examples, avoiding passing /examples as a path with each request. A GET or POST request using "" as a URL would be valid in this case.

Describe the solution you'd like

  • Is it possible for the request builder to accept empty strings but fail for undefined?
  • Or maybe some config option could be introduced to intentionally opt out of the fast failure.
  • An obscure solution would be to additionally pass the baseURL as url to axios.create, but currently the presence of the URL is checked before merging the configs. Moving the if (!config.url) { part down after the config merge would allow this escape hatch.

I'd be happy to submit a PR if desired.

Describe alternatives you've considered

  • Changing the respective occurrences in my code.
  • Using a slash or a space as a URL would work in many cases but it would also introduce trailing slashes.

Additional context

@Atrox
Copy link

Atrox commented Jan 20, 2022

I have also just encountered this problem, in that exact scenario as described above.

@danthul
Copy link

danthul commented Jan 21, 2022

I'd prefer not to pass the url at all. None of my requests have a url because they all go to a graphql server with the same url, i.e. myapp.herokuapp.com/graphql. I set up the baseURL in defaults and don't want or need to set a url on each request. Maybe a config option to ignore the missing url?

@jishidaaaaa
Copy link

I need to change #3916 to upgrade to node14, but on the other hand, I can't raise the version to 0.25.0 because of the this problem. Please fix it as soon as possible.

@jasonsaayman
Copy link
Member

Thanks for this, I am pretty conflicted on my final stance on this issue. On one side of the argument one could say that baseURL is exactly that, the base URL, therefore no path ie. {subdomain}.{domain}.{...extension}. However I can see the utility when one has only a single URL like @danthul.

It would be nice to keep everyone happy, so I would like to give this some more thought come up with a solid solution to this, I will revert back after the weekend with my ideas.

itsokto added a commit to itsokto/duck-bot-node that referenced this issue Jan 25, 2022
@wafuwafu13
Copy link
Contributor

I write #3791, but I didn't given it enough thought. I agree to revert.

@jeanlescure
Copy link

My dev team was blocked from working yesterday due to this. We were getting the error:

error: Error: Provided config url is not valid

We have not one, but a very considerable amount of cases in our code where baseUrl was the default and url was not passed to the request config.

I just spent an all-nighter debugging a monorepo with multiple large apps going down every nook and cranny, until I tested a different branch where what we were trying to fix just worked.

The only noticeable difference? Axios v0.24.0 vs v0.25.0 😕

@adavidarevalo
Copy link

also affected by this, please revert

@hectwor
Copy link

hectwor commented Jan 25, 2022

I have the same problem, please revert to avoid problems

@iabellom
Copy link

also affected by this, please revert

@2huBrulee
Copy link

😔 was blocked by this, please revert this change

@jasonsaayman
Copy link
Member

jasonsaayman commented Jan 26, 2022

I will revert this for now, but in V1 this will no longer be the default. In v1 baseUrl will be a base url and url will have to be filled. This will probably break a lot of projects, however I believe axios needs to be a bit more strict and the code bases implementing maybe need to be a bit more verbose.

@Atrox
Copy link

Atrox commented Jan 26, 2022

Thank you for reverting ❤️.
If possible, could you rethink changing this in v1? It really seems like a valid use case to avoid repetition. :)

@dubst3pp4
Copy link

Thanks for reverting :-) Why not make the url param optional? If provided, it must not be empty, but if it is not provided, just the baseUrl is used...

@felixh10r
Copy link
Author

Thanks! I agree that existing codebases shouldn't be taken too much into consideration when going forward with the project, but the fact that, judging from the reactions on this issue, the "empty URL" use-case obviously occurs a lot may justify some kind of escape hatch down the line. Allowing empty strings as url may still mean touching every service file, but at least it doesn't require changing application logic 🤔

@jasonsaayman
Copy link
Member

We could potentially change the name to path, then make it not required, then this might make more sense, people can then implement baseUrl and leave out path. Does this sound like a better solution?

We can also use a mapper to allow some backward compatibility with url to start off with and then add warnings and remove.

@jeanlescure
Copy link

I think you're hitting all the right keys with this last comment @jasonsaayman 🎉

Introducing breaking changes in a minor or patch version completely goes against the spirit of semantic versioning, but moving this change to v1, it's precisely what major version changes are for.

Personally, first time I used an axios instance with a baseUrl, when I tried using request, I looked for path first when writing its config, so I'd personally agree that it makes more logical sense that way. I think Axios v1 should be more instance focused, it is really a much better design pattern and time-saver to have a single source of truth of the baseUrl and then just append a path to it when needed anywhere else in the code.

Also, in regards to allowing to use a mapper for backwars compatibility in v1, that's a good way to make adoption of v1 easier, which in the long run means less time maintaining v0 both for users of axios as well as maintainers.

Thanks for the awesome work @jasonsaayman 🙌🏼 😄

@jasonsaayman
Copy link
Member

Changes made with #4426 will release soon

@jishidaaaaa
Copy link

Thanks for the fix.

@joelbourbon
Copy link

Hey!

Just a friendly reminder, that if this ever gets put back in, it would be nice to have the types follow to detect those issues before hand in TypeScript.

Removing the optional of url here:

url?: string;

Thanks,

marcolink added a commit to contentful/contentful.js that referenced this issue Feb 11, 2022
Bumping axios to it's latest version [requires](axios/axios#4407) to not send empty strings as url.
Therefore we had to adjust the intetral execution of `getSpace`
marcolink added a commit to contentful/contentful.js that referenced this issue Feb 11, 2022
Bumping axios to it's latest version [requires](axios/axios#4407) to not send empty strings as url.
Therefore we had to adjust the internal execution of `getSpace`
@vernou
Copy link

vernou commented Apr 27, 2022

What's new?

@vernou
Copy link

vernou commented Sep 28, 2022

This pull-request #4426 resolved the problem.
It's available from the version 0.26.0.

The ticket can be closed?

@lokucrazy
Copy link

https://github.com/axios/axios/blob/v1.x/index.d.ts#L482

all the get, post, put, delete etc. still require a url string, shouldn't it also be url?: string

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

No branches or pull requests