-
Notifications
You must be signed in to change notification settings - Fork 541
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: Convert logical nullish assignment for backward compatibility #1625
fix: Convert logical nullish assignment for backward compatibility #1625
Conversation
Is it possible to add a test? |
lib/fetch/headers.js
Outdated
get [kHeadersSortedMap] () { | ||
this[kHeadersList][kHeadersSortedMap] ??= new Map([...this[kHeadersList]].sort((a, b) => a[0] < b[0] ? -1 : 1)) | ||
this[kHeadersList][kHeadersSortedMap] ?? (this[kHeadersList][kHeadersSortedMap] = new Map([...this[kHeadersList]].sort((a, b) => a[0] < b[0] ? -1 : 1))) | ||
return this[kHeadersList][kHeadersSortedMap] |
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 (!this[kHeadersList][kHeadersSortedMap]) {
this[kHeadersList][kHeadersSortedMap] = new Map([...this[kHeadersList]].sort((a, b) => a[0] < b[0] ? -1 : 1)))
}
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.
Is this for code clarity?
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.
yes
I double checked and these lines are already covered by existing tests |
Codecov Report
@@ Coverage Diff @@
## main #1625 +/- ##
==========================================
- Coverage 94.93% 94.89% -0.05%
==========================================
Files 50 50
Lines 4798 4799 +1
==========================================
- Hits 4555 4554 -1
- Misses 243 245 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 main issue is that we don't have tests for regressions, which unfortunately are very hard to make.
…odejs#1625) * fix: remove logical nullish assignment * more readable code
Was a new if (lowercaseName === 'set-cookie') {
this.cookies ??= []
this.cookies.push(value)
} Edit: Seems like it was introduced in 5.15.0 |
??= has been supported since node 15, I recommend updating node to v18 or v20 |
Yea, don't have any control over the target, unfortunately. We've downgraded to 5.14.0 until who knows when. Thanks for the reply. |
…odejs#1625) * fix: remove logical nullish assignment * more readable code
Motivation
Backward compatibility
??=
is only supported fromecmaVersion: 2021
Solution
Converted logical nullish assignment
x ??= y
tox ?? (x = y)
(Ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_nullish_assignment)
Related Issue
Closes #1479