Repository navigation
fix(helpers): return relativeURL when baseURL is missing in combineURLs - #11343
HimanshuSardana wants to merge 5 commits into
Conversation
Return relativeURL when baseURL is undefined, null, or empty instead of throwing TypeError reading length. Mirrors existing !relativeURL guard. Related to axios#11282.
|
Note for maintainers: this PR was created with AI assistance. I tried to apply the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
 [Low impact] No outstanding findings block merging this change. Both previous repository-requirement findings have been addressed. Summary
Reviews (4) · Last reviewed commit: "Merge remote branch to pick up v1.x upda..." · Reviewed by Greptile |
There was a problem hiding this comment.
PR #11343 - fix(helpers): return relativeURL when baseURL is missing in combineURLs
Repo: axios/axios Author: @HimanshuSardana Base: v1.x Files: 3 **+12/-0
CI: 4 success; no build/test workflow result is present in this snapshot. Mergeability: mergeable. Reviewed head: 2bae29c7026c6773c5eb2d5e3e6ee5d4235c4f41.
Repo Profile
Axios is an npm JavaScript HTTP client for browsers and Node, with XHR/Fetch/HTTP adapters, default and named exports, interceptors, headers/errors, and paired ESM/CJS declarations. The target uses Vitest plus browser and package/module tests; v1 packaging covers older Node CJS runtimes as well as current ESM, Bun and Deno. Contributions preserve transport/security boundaries, add regression coverage and prerelease records, and defer unreleased user docs to PRE_RELEASE_DOCS.md. This review uses the actual target-branch guidance and files, not the separate local v0 checkout.
Verdict
Block — Concrete source/test/API issues below must be resolved before merge.
Change Explanation And User Impact
This early !baseURL return fixes undefined/null bases with a supplied relative URL, but also intercepts existing empty-string inputs before normal joining. An expanded executed matrix found combineURLs("", "users") changes from "/users" on the target to "users" here; combineURLs("", undefined) changes from "" to undefined. These affect direct/unsafe-helper consumers even though ordinary request assembly often bypasses the helper for empty bases. The original /users-only probe missed this boundary. Newly opened #11354 uses a nullish guard after the missing-relative check and preserves both existing results. The posted comparison is corrected on that evidence.
Validation scope: source/diff and regression-test review. Examples explicitly marked executed used isolated source snapshots on Node 24.18.0 with the workspace dependency installation; these were focused checks, not a clean install, full CI matrix, or full test-suite run. Author-reported test results were not treated as independently executed.
Competing PRs
Search: axios/axios open PRs, including drafts; full open-list scan plus combineURLs in:title,body, 11282 in:title,body. Complementary searches were run across related APIs/issues; no query reached the 100-result cap and no search failed. The five explicitly excluded PRs were not reviewed or modified in this batch.
Compared #11354, #11343, #11307, #11308, #11283. The expanded empty-base/unrooted matrix changes the earlier preference: #11354 preserves combineURLs("", "users") === "/users" and missing-relative behavior. #11343/#11283 return "users" for that existing input; #11308 also strips an existing root slash; #11307 leaves null unfixed. #11286 expands the public API and is a separate proposal. The two new candidates were included in refreshed open-PR searches before closure.
Preferred PR: #11354 has the narrow nullish guard and preserves the existing empty-base behavior; it still needs a prerelease entry and should gain explicit empty-base regression assertions.
Unique contributions: Preserve the empty-base/rooted-relative regression and release rationale from #11343 with attribution; decide the distinct both-absent contract in #11283 explicitly. The undefined case from #11307 is represented in #11354.
Action: Select #11307 and #11308 for closure in favor of #11354 after final refresh. Keep #11343 and #11283 open pending compatibility/test consolidation; #11286 is independent. No closure is claimed at the time of this review.
Project Fit
North star: Reliable HTTP transport and configuration across supported JavaScript environments, without embedding application-specific policy.
Benefits: Fixes truly missing bases, but needs to preserve the existing empty-base contract.
Tradeoffs: Treating empty and absent bases identically rewrites an existing URL-building result. The narrower nullish guard avoids that compatibility cost.
Fit: Strong — The underlying problem belongs in the transport, serializer, declarations or their maintenance tests.
Adversarial Review
Strongest objection: Preserve existing empty-base URL joining
Abuse/failure modes checked: Compared all five candidates and executed rooted, unrooted and absent relative values across undefined/null/empty bases. The tests in this PR cover only the rooted empty-base case, which does not expose the regression.
Result: Confirmed issue(s) are listed below and attached inline.
Disposition
Continue
Address the specific blockers below; no unrelated scope or complementary PR should be closed.
Semver
major — Changes existing empty-base helper results on the v1 line.
Breaking Changes
lib/helpers/combineURLs.js — empty base plus users changes from /users to users. Preserve the existing result with the narrower nullish guard in #11354; this is separate from avoiding null/undefined exceptions.
Documentation
Correct the prerelease entry: empty bases did not throw in the existing implementation. Preserve their existing results rather than describing the changed semantics as a compatible crash fix.
Performance And Regression Risk
Hot path touched: Yes — request/response processing or its shared helpers.
Performance concern: No unbounded new work identified in the changed path. Treating empty and absent bases identically rewrites an existing URL-building result. The narrower nullish guard avoids that compatibility cost.
Regression risk: medium — demonstrated compatibility/failure case requires correction.
Evidence requested: A fixed reproduction plus the relevant latest-head CI checks.
Blocking Issues
lib/helpers/combineURLs.js:12— An expanded executed compatibility check foundcombineURLs("", "users")returns/userson the target butusershere. Those resolve to different paths for a helper consumer under a non-root document URL. The earlier rooted-relative probe did not expose this. Guard only null/undefined and retain the existing missing-relative handling, as #11354 does; add explicit empty-base tests. This corrects my earlier assessment of this boundary.- Latest-head build/test validation is unavailable in the returned GitHub checks. Run or attach the repository build, relevant regression suite and required package/type/browser checks before merge; review/security bots alone do not establish that they pass. This is a validation hold, not an invented source defect.
Non-Blocking Suggestions
- Consolidate the correct nullish guard with #11354 and preserve this PR's empty-base regression and prerelease rationale with attribution.
Looks Good
- Compared all five candidates and executed rooted, unrooted and absent relative values across undefined/null/empty bases. The tests in this PR cover only the rooted empty-base case, which does not expose the regression.
| * @returns {string} The combined URL | ||
| */ | ||
| export default function combineURLs(baseURL, relativeURL) { | ||
| if (!baseURL) { |
There was a problem hiding this comment.
[P2] Preserve existing empty-base URL joining
An expanded executed compatibility check found combineURLs("", "users") returns /users on the target but users here. Those resolve to different paths for a helper consumer under a non-root document URL. The earlier rooted-relative probe did not expose this. Guard only null/undefined and retain the existing missing-relative handling, as #11354 does; add explicit empty-base tests. This corrects my earlier assessment of this boundary.
The previous !baseURL guard also intercepted empty-string bases, changing
existing results: combineURLs('', 'users') went from '/users' to 'users'
and combineURLs('', undefined) went from '' to undefined. Those resolve to
different paths for direct helper consumers.
Narrow the guard to null/undefined and place it after the existing
!relativeURL check so both existing empty-base results are preserved.
Add explicit empty-base regression assertions (the rooted '/users' case
did not discriminate) and correct the changelog, which claimed empty bases
threw.
Verified against the target implementation across a 56-case matrix of
bases x relative values: the only differences are the 10 previously
throwing undefined/null-base calls, which now return the relative URL.
|
Thanks for catching the empty-base regression, @jasonsaayman — you were right, my first Fixed in 9280ef7:
Evidence:
Happy to consolidate with #11354 however you prefer — my changelog entry and empty-base tests are yours to reuse with attribution if you go with that one instead. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressing the two P2s from the latest Greptile review: 1. Required checks were skipped — done. This is now recorded in the PR Validation section: 2. Release note lacks PR link — fixed in e84b489. The entry now reads "(#11343, related to #11282)". Note: this branch also picked up the v1.x merge (1.21.0 release prep) from the remote — the entry survived the merge; only lockfile/version files changed alongside it. |
Summary
Direct calls to
combineURLs(undefined, '/users')crashed withTypeError: Cannot read properties of undefined (reading 'length')atlib/helpers/combineURLs.js:16. This adds anull/undefinedguard (placed after the existing!relativeURLcheck) that returnsrelativeURL, mirroring the existing missing-relative handling. Empty-string bases keep their existing joining behavior.Linked issue
Related to #11282
Changes
lib/helpers/combineURLs.js: returnrelativeURLwhenbaseURLisnullorundefinedtests/unit/helpers/combineURLs.test.js: regression tests forundefined/nullbaseURL plus explicit empty-base casesPRE_RELEASE_CHANGELOG.md: Bug Fixes entryValidation
npm run lint— 0 errors, 4 warnings in 2 test files I did not touch (http.test.js,prototypePollution.test.js; pre-existing)npm run build— success (all bundles created)npx vitest run --project unit tests/unit/helpers/combineURLs.test.js— 8 passednpm run test:vitest:unit— 1700 passed, 2 failed:fetch.test.jsSymbol.iterator failure reproduces on the clean tree (pre-existing);http.test.jsHTTP2 session failure passes when run alone (flaky/network). Neither is related to this change.undefined/null-base calls changed (now return the relative URL); all empty-base results identicalaction_required(fork PR, needs maintainer approval to run) — no build/test workflow results yetCompatibility and documentation
Patch-level bug fix, no breaking behavior. Internal
buildFullPathalready guards withif (baseURL && ...)so its behavior is unchanged. No PRE_RELEASE_DOCS.md or index.d.ts/index.d.cts update needed (no public API change).Checklist
PRE_RELEASE_CHANGELOG.mdentry added or updated (or AGENTS.md's automated release-preparation exception explained)