fix: remove duplicate baseUrl in axios client

[Ben-Pfirsich]

No description provided.

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 86f2e0c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

@ben-pietsch is attempting to deploy a commit to the Hey API Team on Vercel.

A member of the Team first needs to authorize it.

[Ben-Pfirsich]

Hi @mrlubos,

this is my attempt to fix the issue I described in #2620

I was not entirely sure how to structure the tests.

How do I proceed?

[mrlubos]

@Ben-Pfirsich you'd want to run pnpm test:update to update snapshots and make CI pass, but let's make sure this doesn't break things for people who might be building their URLs with absolute base URLs

[Ben-Pfirsich]

Hi @mrlubos,

i cannot get the project to build locally. The pnpm test:update also always fails at some point.

× index > downloads and parses v2 without issues 10015ms
@test/openapi-ts:test:update: → Test timed out in 10000ms.

[mrlubos]

Did you build the library?

[Ben-Pfirsich]

Yes, but when I try to the build in the root folder, I always get some different error. I am able to build some of the subfolders.
Do I need to run everything in the root folder?

[mrlubos]

@Ben-Pfirsich hard to say without knowing the error message. I'd start by building codegen-core, openapi-ts, potentially custom-client for tests. I think you also want to build @config/vite-base if this is the first time you're setting it up.

[Ben-Pfirsich]

@mrlubos I will try to build it again tomorrow. Did you see my answer to your comment on the code?

[Ben-Pfirsich]

Hi @mrlubos,

I though of another option, that would maybe leave all the current config object alone.
When calling axios, you could check if the baseURL is already included in (or startsWith) the url and react to that. I think when passing a baseURL as an empty string it would even override the axios.defaults baseURL, and duplicate configurations would not result a duplicate in the called url.

I do think this is more confusing from a user perspective though.

     // set baseUrl to empty string, when it is already included in URL
      const baseUrl = (typeof opts.baseURL === 'string' && url.includes(opts.baseURL)) ? '' : opts.baseURL;

      const response = await _axios({
        ...optsWithoutAuth,
        baseURL: baseUrl as string,
        data: getValidRequestBody(opts),
        headers: opts.headers as RawAxiosRequestHeaders,
        // let `paramsSerializer()` handle query params if it exists
        params: opts.paramsSerializer ? opts.query : undefined,
        url,
      });

[mrlubos]

Explain why is it more confusing? Would there be any breaking change or weird behavior? Just looking at it quickly, I think it makes sense and could be applied across all clients

[Ben-Pfirsich]

Explain why is it more confusing? Would there be any breaking change or weird behavior? Just looking at it quickly, I think it makes sense and could be applied across all clients

Hi @mrlubos ,

i think there could be some edge cases, where is does not work.

If someone has a (weird) url like this:

/example/example/some-api/v1

where the baseURL is "/example", this logic would wrongly assume that the baseURL is already included in this URL: "/example/some-api/v1.

I think it might be more robust to make sure that the buildUrl function always includes the baseUrl if present and then always disable the baseUrl that is already in the axios instance.

Something like this (previously the baseURL from the axios instance was not included):

export const buildUrl: Client['buildUrl'] = (options) =>
  getUrl({
    baseUrl: options.baseURL ?? options.axios?.defaults.baseURL as string,
    path: options.path,
    // let `paramsSerializer()` handle query params if it exists
    query: !options.paramsSerializer ? options.query : undefined,
    querySerializer:
      typeof options.querySerializer === 'function'
        ? options.querySerializer
        : createQuerySerializer(options.querySerializer),
    url: options.url,
  });

      const response = await _axios({
      ...optsWithoutAuth,
      baseURL: '', // the baseURL is already included in `url`
      data: getValidRequestBody(opts),
      headers: opts.headers as RawAxiosRequestHeaders,
      // let `paramsSerializer()` handle query params if it exists
      params: opts.paramsSerializer ? opts.query : undefined,
      url,
    });

I think this is similar to what you suggested before.

[mrlubos]

If this works I'm all for it!

[Ben-Pfirsich]

If this works I'm all for it!

@mrlubos ,

I will try to implement this, but I just noticed that the buildUrl and getUrl do not have any handling for absolute url inputs. When passing an absolute url it will still try to append it to the baseUrl. Is this the expected behaviour, or do I have include a guard?

[mrlubos]

What do you mean by "it will still try to append it to the baseUrl"? The implementation looks like this

export const getUrl = ({
  baseUrl,
  path,
  query,
  querySerializer,
  url: _url,
}: {
  baseUrl?: string;
  path?: Record<string, unknown>;
  query?: Record<string, unknown>;
  querySerializer: QuerySerializer;
  url: string;
}) => {
  const pathUrl = _url.startsWith('/') ? _url : `/${_url}`;
  let url = (baseUrl ?? '') + pathUrl;
  if (path) {
    url = defaultPathSerializer({ path, url });
  }
  let search = query ? querySerializer(query) : '';
  if (search.startsWith('?')) {
    search = search.substring(1);
  }
  if (search) {
    url += `?${search}`;
  }
  return url;
};

It should return a valid absolute URL if possible. In your case, it won't do that, which is also fine. The problem stems from the internal Axios implementation that incorrectly duplicates the base. The point of this function is to construct the final URL we'll use in the request. It seems the easiest solution might be to simply exclude base URL from the Axios call since we construct it ourselves anyway

[Ben-Pfirsich]

@mrlubos,
it looks to me like passing baseUrl=https://example.com and url=https://api.example.com/users to the getUrl function would return https://example.com/https://api.example.com/users.

Am I overlooking something?

[mrlubos]

@Ben-Pfirsich that's correct but how would you end up with domain in url which is the path to the resource?

[Ben-Pfirsich]

@Ben-Pfirsich that's correct but how would you end up with domain in url which is the path to the resource?

@mrlubos
If that never happens, than that is fine. That is what I wanted to know.

[Ben-Pfirsich]

Hi @mrlubos,

I just pushed my second attempt. A few notes:

• I am not sure, if the input type of the baseURL can be changed (i.e. removing the omit of axios)
• Inside the client test I added some tests to confirm how axios builds its URLs, this should probably not be kept
• I am still unable to build the complete project from this device. Same problem with the test:update. I will try it again on a different computer in a different network

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 23.83%. Comparing base (65d16b3) to head (86f2e0c).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2624   +/-   ##
=======================================
  Coverage   23.83%   23.83%           
=======================================
  Files         379      379           
  Lines       36930    36951   +21     
  Branches     1626     1630    +4     
=======================================
+ Hits         8803     8809    +6     
- Misses      28114    28129   +15     
  Partials       13       13           

Flag	Coverage Δ
unittests	23.83% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/codegen-core@2624

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2624

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2624

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2624

commit: 86f2e0c

[mrlubos]

Seems you figured it out. Do you know what was the problem?

[Ben-Pfirsich]

@mrlubos ,
It seems like the company network I was in was blocking some URLs. Building it on my private computer worked just fine.

[Ben-Pfirsich]

@mrlubos,
Did you have a chance to look at the changes I made? Is there anything you would want me to do differently?