-
Notifications
You must be signed in to change notification settings - Fork 716
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
fix: avoid overwriting the default values of options with undefined #1018
fix: avoid overwriting the default values of options with undefined #1018
Conversation
async function mergeConfig (options, context, gitRawCommitsOpts, parserOpts, writerOpts, gitRawExecOpts) { | ||
let configPromise | ||
let pkgPromise | ||
|
||
options = omitUndefinedValueProps(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here omits props whose value is undefined to avoid overwriting the default values with undefined.
@dangreen I would appreciate your review 🙏 |
…s of options with undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Please fix CI errors and provide tests for your changes.
f1f5a1f
to
c202a18
Compare
@@ -0,0 +1,61 @@ | |||
'use strict' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file names (merge-config.spec.js
and index.spec.js
) are based on other packages such as conventional-commits-parser, conventional-changelog-writer.
expect(options).to.include(defaultOptions) | ||
}) | ||
|
||
it('should return default options when undefined value is passed', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If props whose value is undefined are not omitted, this test will fail since the default values will be overwritten with undefined.
095c28a
to
229622c
Compare
function omitUndefinedValueProps (obj) { | ||
if (obj === null || typeof obj !== 'object') { | ||
return {} | ||
} | ||
const omittedObj = Object.entries(obj).filter(([, value]) => value !== undefined) | ||
return Object.fromEntries(omittedObj) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's optimize it to one loop
function omitUndefinedValueProps (obj) { | |
if (obj === null || typeof obj !== 'object') { | |
return {} | |
} | |
const omittedObj = Object.entries(obj).filter(([, value]) => value !== undefined) | |
return Object.fromEntries(omittedObj) | |
} | |
function omitUndefinedValueProps (obj) { | |
if (!obj) { | |
return {} | |
} | |
const omittedObj = {} | |
for (const key in obj) { | |
if (obj[key] !== undefined) { | |
omittedObj[key] = obj[key] | |
} | |
} | |
return omittedObj | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimized in ddf323b
8f118d5
to
ddf323b
Compare
@dangreen Thanks for the quick release! |
The default values of options (
append
,releaseCount
,skipUnstable
...) appear to be not working as intended.If
releaseCount
option is not specified, theoptions
object initially has the propertyreleaseCount: undefined
, and then the default valuereleaseCount: 1
looks like to be assigned, but actuallyreleaseCount: undefined
is assigned again due to merging...options
(which hasreleaseCount: undefined
) with the default values.conventional-changelog/packages/conventional-changelog-core/lib/merge-config.js
Lines 66 to 95 in 11195f2
This problem has been introduced by #959.