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

Logging with baseUrl #42

Closed
wants to merge 3 commits into from
Closed

Logging with baseUrl #42

wants to merge 3 commits into from

Conversation

tenorok
Copy link

@tenorok tenorok commented Mar 4, 2020

Hello, I noticed that the URL is logging not completely when using baseUrl config property.

For example:

axios.create({
    baseURL: 'https://example.com/path',
}).get('/to');

Before:

axios 200 OK (GET /to) +325ms

After:

axios 200 OK (GET https://example.com/path/to) +325ms

@Gerhut
Copy link
Owner

Gerhut commented Mar 5, 2020

Hi @tenorok

Thanks for your contribution!

Could you please fix the code style to match standardjs?

Moreover, it seems the CI is broken, maybe we should use the url toolkit instead of path, feel free to have a try

@Gerhut
Copy link
Owner

Gerhut commented Mar 5, 2020

Actually, it's a breaking change in axios@0.19.1, which won't merge baseURL into config.url in adapters. After you finish this change, it's better to test it in both 0.19.0 and 0.19.1.

Appreciate again

@coveralls
Copy link

coveralls commented Mar 6, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 6f89ab6 on tenorok:add-baseUrl into 7d29d83 on Gerhut:master.

@tenorok
Copy link
Author

tenorok commented Mar 6, 2020

I fixed code style and move to use url module. Also created a unit test and successful ran it with axios@0.19.[0,1,2] if I correct understand you.

@Gerhut
Copy link
Owner

Gerhut commented Mar 7, 2020

Would you try { baseURL: 'http://example.com/foo', url: '/bar' } ?

@tenorok
Copy link
Author

tenorok commented Mar 9, 2020

You absolutely right and now I see what did you mean. I think that most reasonably use functions from the axios itself, but for versions earlier than 0.19.1 just the copied function buildFullPath().
I created tests for different cases.

If everything okay let me know and I will squash the commits.

@Gerhut
Copy link
Owner

Gerhut commented Mar 10, 2020

Still incorrect with { baseURL: '/foo', url: '/bar' } in axios@0.19.0...

OK, due to such a horrible breaking change in axios@0.19.1 (even a patch version), I think we'd better separate this library into 2 different versions:

  • 0.6 for "axios": ">=0.13.0 <=0.19.0"
  • 0.7 for "axios": ">=0.19.1"

I am working on release 0.6.3 to limit axios <=0.19.0 in peerDeps. Would you clean up your code to support only axios >=0.19.1? Including update package.json and .travis.yml and I will merge your code for the 0.7.0 version. It seems we can just use the internal helpers to deal with baseURL and url configs so just keeping a simple test case with baseURL will be OK.

Thanks for your hard work!

Edit: see below

@Gerhut
Copy link
Owner

Gerhut commented Mar 10, 2020

Oh, after some investigation, it seems not work well even after 0.17.0 (axios/axios#950)

So actually there are 3 behaviors with url and baseURL in axios:

  1. axios@<0.17.0: merged in request, merged in response
  2. axios@<0.19.1: not merged in request, merged in response
  3. axios@>=0.19.1: not merged in request, not merged in response

Hence, we really need a feature detection but not a version detection


Got an approach: we could make use of the bold behavior, and drop axios@<0.17.0 support. Which means manually build URL in request, and ignore the one in response.

Anyway, this will be into a major (0.7.0) change, will continue in #43

@Gerhut Gerhut closed this Mar 16, 2020
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.

None yet

3 participants