Refactor mergeConfig without utils.deepMerge (#2844)#3
Open
MitchLewis930 wants to merge 1 commit intopr_023_beforefrom
Open
Refactor mergeConfig without utils.deepMerge (#2844)#3MitchLewis930 wants to merge 1 commit intopr_023_beforefrom
MitchLewis930 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>
There was a problem hiding this comment.
Pull request overview
This PR refactors the mergeConfig function to eliminate the use of utils.deepMerge, consolidating merge functionality into a single utils.merge implementation with improved handling of plain objects, arrays, and null/undefined values.
Changes:
- Removed
utils.deepMergefunction and updatedutils.mergeto handle deep merging with proper cloning - Added
utils.isPlainObjectto distinguish plain objects from arrays and other object types - Refactored
mergeConfigto use a newgetMergedValuehelper function with distinct merge strategies for different config key types - Added new
directMergeKeyscategory for properties likevalidateStatusthat should merge even when explicitly set toundefined
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/utils.js | Removed deepMerge function, added isPlainObject helper, updated merge to properly clone nested objects and arrays |
| lib/core/mergeConfig.js | Refactored to use new merge strategy with getMergedValue helper and directMergeKeys category |
| lib/core/Axios.js | Updated method shortcuts to use mergeConfig instead of utils.merge |
| lib/core/dispatchRequest.js | Removed null header deletion logic (now handled in merge) |
| test/specs/utils/merge.spec.js | Added tests for cloning behavior with nested objects, arrays, and null/undefined handling |
| test/specs/utils/isX.spec.js | Added tests for new isPlainObject function |
| test/specs/utils/deepMerge.spec.js | Removed obsolete test file for deleted deepMerge function |
| test/specs/core/mergeConfig.spec.js | Added comprehensive tests for different merge strategies |
| test/specs/headers.spec.js | Updated test to reflect new null/undefined header handling behavior |
| test/specs/requests.spec.js | Added tests for validateStatus with null/undefined values |
| config[prop] = config2[prop]; | ||
| } else if (typeof config1[prop] !== 'undefined') { | ||
| config[prop] = config1[prop]; | ||
| utils.forEach(directMergeKeys, function merge(prop) { |
There was a problem hiding this comment.
The callback function is named 'merge' which shadows the outer scope's utils.merge function and could cause confusion. Consider renaming to 'directMerge' or 'mergeDirectKey' to avoid ambiguity.
Suggested change
| utils.forEach(directMergeKeys, function merge(prop) { | |
| utils.forEach(directMergeKeys, function directMerge(prop) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_023