Refactor mergeConfig without utils.deepMerge (#2844)#3
Refactor mergeConfig without utils.deepMerge (#2844)#3MitchLewis930 wants to merge 1 commit intopr_023_beforefrom
Conversation
* Adding failing test * Fixing axios#2587 default custom config persisting * Adding Concat keys and filter duplicates * Fixed value from CPE * update for review feedbacks * no deepMerge * only merge between plain objects * fix rename * always merge config by mergeConfig * extract function mergeDeepProperties * refactor mergeConfig with all keys, and add special logic for validateStatus * add test for resetting headers * add lots of tests and fix a bug * should not inherit `data` * use simple toString * revert axios#1845 Co-authored-by: David Tanner <david.tanner@lifeomic.com> Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe pull request refactors axios's merge strategy by introducing new merging helpers ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/specs/headers.spec.js`:
- Around line 60-83: The test "should reset headers by null or explicit
undefined" currently asserts null/undefined values but real XHR coerces these to
strings; update the assertions so they reflect actual runtime behavior: after
calling axios.create(...).post(...) and receiving getAjaxRequest(), use
testHeaderValue to expect 'null' for the headers passed as null (e.g.,
'Content-Type' and 'x-header-a') and 'undefined' for the header passed as
undefined ('x-header-b'), leaving 'x-header-c' as 'c'; reference the
axios.create/post call, getAjaxRequest(), and testHeaderValue to locate and
update the expectations accordingly.
🧹 Nitpick comments (1)
lib/core/mergeConfig.js (1)
77-82: Consider deduplicating otherKeys.When the same custom key exists in both
config1andconfig2, it will appear twice inotherKeys, causingmergeDeepPropertiesto process it redundantly. While this produces correct results (the second pass overwrites with the same value), it's a minor inefficiency.♻️ Optional: deduplicate keys using a filter
var otherKeys = Object .keys(config1) .concat(Object.keys(config2)) - .filter(function filterAxiosKeys(key) { - return axiosKeys.indexOf(key) === -1; - }); + .filter(function filterAxiosKeys(key, index, self) { + return axiosKeys.indexOf(key) === -1 && self.indexOf(key) === index; + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/core/Axios.jslib/core/dispatchRequest.jslib/core/mergeConfig.jslib/utils.jstest/specs/core/mergeConfig.spec.jstest/specs/headers.spec.jstest/specs/requests.spec.jstest/specs/utils/deepMerge.spec.jstest/specs/utils/isX.spec.jstest/specs/utils/merge.spec.js
💤 Files with no reviewable changes (2)
- lib/core/dispatchRequest.js
- test/specs/utils/deepMerge.spec.js
🔇 Additional comments (10)
lib/utils.js (2)
108-121: LGTM! Well-implemented plain object detection.The
isPlainObjectfunction correctly identifies plain objects by first checking the[object Object]tag and then verifying the prototype is eithernullorObject.prototype. This properly excludes arrays, class instances, and objects created with non-standard prototypes.
279-287: LGTM! Merge logic correctly handles all value types.The updated
assignValuefunction properly:
- Recursively merges when both values are plain objects
- Clones plain objects to avoid shared references
- Clones arrays with
slice()to prevent mutation- Directly assigns primitives and non-plain objects
test/specs/utils/isX.spec.js (1)
54-59: LGTM! Good test coverage for isPlainObject.The tests cover the key edge cases: plain empty object, array, null, and objects with non-standard prototypes via
Object.create({}).test/specs/utils/merge.spec.js (1)
42-83: LGTM! Comprehensive test coverage for merge edge cases.These tests properly validate:
- Reference isolation for nested objects (line 53)
- Graceful handling of null/undefined arguments
- Proper null preservation in merged results
- Array replacement and cloning behavior
The use of
not.toBe()assertions correctly verifies that cloning produces new references rather than sharing objects.lib/core/mergeConfig.js (2)
29-38: LGTM! Clean helper for merging values.
getMergedValuecorrectly handles all merge scenarios: deep merge for plain objects, cloning for plain objects and arrays, and direct assignment for primitives.
64-70: Good use ofinoperator for directMergeKeys.Using
prop in config2instead of!utils.isUndefined(config2[prop])correctly allows explicitly settingundefinedas a value (e.g.,{validateStatus: undefined}), which is important for resetting defaults.test/specs/core/mergeConfig.spec.js (2)
103-131: LGTM! Thorough test coverage for valueFromConfig2Keys.Tests properly verify:
- Config2 values are used when present
- Plain objects and arrays are cloned (not shared references)
- Non-plain objects and primitives are passed through directly
250-309: LGTM! Good coverage for directMergeKeys behavior.The tests correctly validate that
validateStatususes theinoperator semantics, including the important case at line 252 where explicitly settingundefinedis preserved in the merged result.lib/core/Axios.js (1)
73-91: LGTM! Correct migration to mergeConfig.Replacing
utils.mergewithmergeConfigensures consistent merge semantics across the codebase. The method-specific values (method,url,data) are invalueFromConfig2Keys, so they correctly override any conflicting values in the user-provided config.test/specs/requests.spec.js (1)
180-220: Good coverage for validateStatus null/undefined edge cases.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| it('should reset headers by null or explicit undefined', function (done) { | ||
| axios.create({ | ||
| headers: { | ||
| common: { | ||
| 'x-header-a': 'a', | ||
| 'x-header-b': 'b', | ||
| 'x-header-c': 'c' | ||
| } | ||
| } | ||
| }).post('/foo', {fizz: 'buzz'}, { | ||
| headers: { | ||
| 'Content-Type': null, | ||
| 'x-header-a': null, | ||
| 'x-header-b': undefined | ||
| } | ||
| }); | ||
|
|
||
| getAjaxRequest().then(function (request) { | ||
| testHeaderValue(request.requestHeaders, 'Content-Type', null); | ||
| testHeaderValue(request.requestHeaders, 'x-header-a', null); | ||
| testHeaderValue(request.requestHeaders, 'x-header-b', undefined); | ||
| testHeaderValue(request.requestHeaders, 'x-header-c', 'c'); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test && pwd && ls -la specs/ | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 1305
🏁 Script executed:
cat -n specs/headers.spec.js | sed -n '1,100p'Repository: Signal65/axios-CodeRabbit
Length of output: 122
🏁 Script executed:
# Look for the testHeaderValue function definition
rg -n "testHeaderValue" test/specs/headers.spec.js | head -5Repository: Signal65/axios-CodeRabbit
Length of output: 402
🏁 Script executed:
# Search for testHeaderValue function definition in the codebase
rg -n "function testHeaderValue|const testHeaderValue|testHeaderValue\s*=" test/ --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 143
🏁 Script executed:
cat -n test/specs/headers.spec.js | head -60Repository: Signal65/axios-CodeRabbit
Length of output: 1899
🏁 Script executed:
# Look at the testHeaderValue function implementation
rg -A 10 "^function testHeaderValue" test/specs/headers.spec.jsRepository: Signal65/axios-CodeRabbit
Length of output: 295
🏁 Script executed:
# Check axios header handling - look for how null/undefined headers are processed
rg -n "setRequestHeader|headers\[" lib/adapters/xhr.js | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 241
🏁 Script executed:
# Search for axios header processing logic
fd "headers" lib/ --type f | head -10Repository: Signal65/axios-CodeRabbit
Length of output: 96
🏁 Script executed:
# Look at full xhr adapter to see how headers are filtered
cat -n lib/adapters/xhr.js | sed -n '120,150p'Repository: Signal65/axios-CodeRabbit
Length of output: 1428
🏁 Script executed:
# Look for header filtering logic in utils or elsewhere
rg -B 5 -A 5 "setRequestHeader" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 660
🏁 Script executed:
# Check if there's special handling for null/undefined values in header processing
rg -n "null|undefined" lib/adapters/xhr.js | grep -i headerRepository: Signal65/axios-CodeRabbit
Length of output: 190
🏁 Script executed:
# Look for axios header defaults and processing
cat -n lib/core/dispatchRequest.js | head -50Repository: Signal65/axios-CodeRabbit
Length of output: 1659
🏁 Script executed:
# Look for where null/undefined headers might be filtered out
rg -n "null|undefined" lib/core/mergeConfig.js | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 398
🏁 Script executed:
# Search for header merging/normalization logic
rg -B 3 -A 3 "forEach.*header" lib/core/ --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Look for utils.merge to understand how headers are merged
cat -n lib/utils.js | sed -n '1,100p'Repository: Signal65/axios-CodeRabbit
Length of output: 3234
🏁 Script executed:
# Find merge function implementation
rg -n "merge.*function|merge.*=" lib/utils.js | head -5Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Search for merge function definition
rg -A 20 "function merge" lib/utils.js | head -50Repository: Signal65/axios-CodeRabbit
Length of output: 581
🏁 Script executed:
# Look for where headers might be filtered for null values before setRequestHeader
rg -B 10 "setRequestHeader" lib/adapters/xhr.js | head -40Repository: Signal65/axios-CodeRabbit
Length of output: 777
🏁 Script executed:
# Search more broadly in xhr adapter for null/undefined handling
cat -n lib/adapters/xhr.js | sed -n '100,140p'Repository: Signal65/axios-CodeRabbit
Length of output: 1735
🏁 Script executed:
# Look at how headers are built/normalized in defaults or mergeConfig
rg -n "headers\s*=" lib/core/mergeConfig.js | head -10Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Look for where null/undefined headers might be filtered - search in header normalizing functions
fd -e js "header" lib/ --type f | xargs wc -l | sort -nRepository: Signal65/axios-CodeRabbit
Length of output: 152
🏁 Script executed:
# Look at what's in lib/core/normalizeHeaderName
ls -la lib/core/Repository: Signal65/axios-CodeRabbit
Length of output: 820
🏁 Script executed:
# Check if there's any special handling for null values in the header processing before xhr adapter
rg -n "null" lib/core/dispatchRequest.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Let's trace through: look at the full test case including setup
cat -n test/specs/headers.spec.js | sed -n '60,100p'Repository: Signal65/axios-CodeRabbit
Length of output: 1531
🏁 Script executed:
# Look for any header filtering or normalization before the xhr adapter gets them
rg -B 5 -A 5 "null" lib/adapters/xhr.js | head -40Repository: Signal65/axios-CodeRabbit
Length of output: 1318
🏁 Script executed:
# Look at normalizeHeaderName to see if there's header value filtering
cat -n lib/helpers/normalizeHeaderName.jsRepository: Signal65/axios-CodeRabbit
Length of output: 509
🏁 Script executed:
# Search for where null/undefined might be filtered from headers - check mergeConfig
cat -n lib/core/mergeConfig.js | head -80Repository: Signal65/axios-CodeRabbit
Length of output: 3400
🏁 Script executed:
# Look for getMergedValue function which might filter null
rg -B 2 -A 10 "function getMergedValue" lib/core/mergeConfig.jsRepository: Signal65/axios-CodeRabbit
Length of output: 452
🏁 Script executed:
# Check if there's post-processing that filters null/undefined headers
rg -n "delete.*header|requestHeaders\[" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 321
🏁 Script executed:
# Look for where headers might be filtered before being passed to setRequestHeader
# Check if there's any code that removes null/undefined from headers
rg -B 5 "request.setRequestHeader" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 362
🏁 Script executed:
# Check if there's any header cleanup/filtering in the header merging process
rg -n "delete.*headers|headers\[.*\]\s*=" lib/core/dispatchRequest.jsRepository: Signal65/axios-CodeRabbit
Length of output: 108
🏁 Script executed:
# Let's check the actual test - what does it expect to find in requestHeaders?
# The key is: does jasmine.Ajax mock store null as null, or convert to string?
# Let's look for any docs or other tests that might show this behavior
rg -B 3 -A 3 "requestHeaders" test/specs/headers.spec.js | head -60Repository: Signal65/axios-CodeRabbit
Length of output: 1451
🏁 Script executed:
# Let's check if there's any header filtering based on null values
rg -n "val.*null|null.*val" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Search for any code that might filter or handle null headers specifically
rg -n "val\s*===\s*null|null.*header|header.*null" lib/ --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 151
🏁 Script executed:
# Check if there's filtering of null values in utils.forEach or similar
rg -B 3 -A 3 "forEach.*requestHeaders|forEach.*headers" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 400
🏁 Script executed:
# Look for where headers might be cleaned up between request building and setRequestHeader call
cat -n lib/adapters/xhr.js | sed -n '1,50p'Repository: Signal65/axios-CodeRabbit
Length of output: 2165
🏁 Script executed:
# Check if axios has a defaults for header handling that might strip nulls
rg -n "headers" lib/defaults.js | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 707
🏁 Script executed:
# Check if there's any filtering before headers reach the xhr adapter
# Look for where config.headers is processed before being passed to the adapter
cat -n lib/core/dispatchRequest.jsRepository: Signal65/axios-CodeRabbit
Length of output: 2555
🏁 Script executed:
# Let's check what happens with null values in the merge process
# Does utils.merge preserve null values?
rg -B 5 -A 15 "function merge" lib/utils.js | head -40Repository: Signal65/axios-CodeRabbit
Length of output: 661
🏁 Script executed:
# Check if there's any header filtering that removes null/undefined values
rg -n "null.*header|header.*null|delete.*header" lib/core/ --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 136
🏁 Script executed:
# Look at test helper to understand how jasmine.Ajax stores headers
cat -n test/__helpers.jsRepository: Signal65/axios-CodeRabbit
Length of output: 118
🏁 Script executed:
find test -name "*helper*" -o -name "*setup*" | head -10Repository: Signal65/axios-CodeRabbit
Length of output: 111
🏁 Script executed:
# Look for getAjaxRequest function
rg -n "getAjaxRequest" test/Repository: Signal65/axios-CodeRabbit
Length of output: 6275
🏁 Script executed:
# Check the complete test file to see what's imported/setup
head -n 30 test/specs/headers.spec.jsRepository: Signal65/axios-CodeRabbit
Length of output: 706
🏁 Script executed:
# Let's search for how jasmine.Ajax stores headers
rg -n "requestHeaders" test/specs/ | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 2168
🏁 Script executed:
# Let's look at the helper file to understand getAjaxRequest and jasmine.Ajax
cat -n test/specs/__helpers.jsRepository: Signal65/axios-CodeRabbit
Length of output: 3692
🏁 Script executed:
# Check if there's any special handling of null/undefined values in header setting
rg -B 5 -A 5 "setRequestHeader" test/ --type js | head -40Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Search for any documentation or comments about null header behavior
rg -i "null.*header|reset.*header" test/specs/ | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 388
🏁 Script executed:
# Look for how axios should handle null headers - check if there's any filtering logic we missed
rg -n "delete.*null|filter.*null|null.*filter" lib/ --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Check if there's any logic to remove null/undefined before calling setRequestHeader
cat -n lib/adapters/xhr.js | sed -n '110,140p'Repository: Signal65/axios-CodeRabbit
Length of output: 1413
🏁 Script executed:
# Let's look for any other tests that verify null/undefined header behavior
rg -B 3 -A 3 "null|undefined" test/specs/headers.spec.jsRepository: Signal65/axios-CodeRabbit
Length of output: 1098
🏁 Script executed:
# Most importantly: check what the actual code does - does it filter null/undefined headers?
# Look for the complete forEach loop
rg -B 2 -A 8 "utils.forEach(requestHeaders" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 161
🏁 Script executed:
# Fix the regex - look for the forEach with requestHeaders more carefully
cat -n lib/adapters/xhr.js | sed -n '124,136p'Repository: Signal65/axios-CodeRabbit
Length of output: 629
🏁 Script executed:
# Check if there's any filtering before headers reach the xhr adapter
# Look at how headers are built in dispatchRequest and before
rg -n "delete.*header" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Look for any special handling of null/undefined in the header processing pipeline
rg -n "null|undefined" lib/core/dispatchRequest.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Let's check if there's any code that strips null values from headers object
rg -B 3 -A 3 "for.*in.*header|forEach.*header" lib/ --type js | grep -A 10 -B 10 "null"Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Look at what would actually be stored in requestHeaders - is there any pre-filtering?
# Let's trace through: when null is passed to setRequestHeader, what does jasmine.Ajax store?
rg -n "requestHeaders" test/specs/__helpers.jsRepository: Signal65/axios-CodeRabbit
Length of output: 361
🏁 Script executed:
# Let me check if there's any logic to handle null/undefined headers removal
# Search for logic that might skip null/undefined values
rg -B 5 -A 5 "val.*null|null.*val|undefined.*val|val.*undefined" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Check if there's any related issue or PR context in comments
rg -i "remove.*header|header.*remove|reset" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Let's see if there's any preprocessing that removes null headers before they reach the adapter
cat -n lib/core/dispatchRequest.js | sed -n '23,50p'Repository: Signal65/axios-CodeRabbit
Length of output: 931
🏁 Script executed:
# Check what axios intends by looking for any related tests or logic for null header handling
rg -n "null.*header" test/specs/ --type js | head -20Repository: Signal65/axios-CodeRabbit
Length of output: 292
🏁 Script executed:
# Critical: check if there's ACTUAL header filtering for null anywhere
rg -n "delete requestHeaders|delete.*header" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 258
🏁 Script executed:
# Let me verify the actual behavior by checking if there's ANY code that filters null headers
# Search for any code that checks val === null or typeof val before using it
rg -n "val\s*===" lib/adapters/xhr.jsRepository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Check if utils.forEach skips null/undefined values
rg -A 20 "function forEach" lib/utils.js | head -30Repository: Signal65/axios-CodeRabbit
Length of output: 626
🏁 Script executed:
# Critical check: Is there filtering logic that removes null headers BEFORE setRequestHeader is called?
# Let me search for any cleanup of headers
rg -B 10 "setRequestHeader" lib/adapters/xhr.js | grep -i "delete\|filter\|null"Repository: Signal65/axios-CodeRabbit
Length of output: 106
🏁 Script executed:
# The key test: does the test actually pass? Let's see if there are any other header tests with null
rg -n "null" test/specs/headers.spec.js | head -15Repository: Signal65/axios-CodeRabbit
Length of output: 354
Clarify semantics for null-valued headers in "reset" test.
The test name implies "reset" behavior (removal/absence), but the assertions expect explicit null values to be passed to and stored by the mock. In real XHR environments, setRequestHeader(key, null) coerces to the string "null", not a null value. Since the code currently passes null values directly to setRequestHeader without filtering, the mock test will succeed but real-world behavior may differ—either clarify the intent (is "reset" meant to be removal or null-passing?), or ensure the test reflects actual runtime behavior.
🤖 Prompt for AI Agents
In `@test/specs/headers.spec.js` around lines 60 - 83, The test "should reset
headers by null or explicit undefined" currently asserts null/undefined values
but real XHR coerces these to strings; update the assertions so they reflect
actual runtime behavior: after calling axios.create(...).post(...) and receiving
getAjaxRequest(), use testHeaderValue to expect 'null' for the headers passed as
null (e.g., 'Content-Type' and 'x-header-a') and 'undefined' for the header
passed as undefined ('x-header-b'), leaving 'x-header-c' as 'c'; reference the
axios.create/post call, getAjaxRequest(), and testHeaderValue to locate and
update the expectations accordingly.
2 participants
PR_023
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.