-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI fix KVv2 in json editor when value is null #27094
Conversation
CI Results: |
Build Results: |
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.
Great test coverage! just one suggestion
// unfortunately in javascript typeof null returns object | ||
// this is due to a "historical js bug that will never be fixed" | ||
// we handle this situation here | ||
if (obj[key] === null) { |
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.
Could we instead move this to its own else if
block before the 'object' check? that would flatten the logic
@@ -30,7 +30,12 @@ export function obfuscateData(obj) { | |||
for (const key of Object.keys(obj)) { | |||
if (Array.isArray(obj[key])) { | |||
newObj[key] = obj[key].map(() => '********'); | |||
} else if (typeof obj[key] === 'object') { | |||
} else if (typeof obj[key] === 'object' && obj[key] === null) { |
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.
You can just check if it === null
} else if (typeof obj[key] === 'object' && obj[key] === null) { | |
} else if (obj[key] === null) { |
and then below just check if it's type object
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.
Thank you! I was fearful that it would hit the second else if again, but testing while it does it doesn't seem to break anything.
Regression PR #24530
Fixes Issue #26709
Note: confirmed that you can in fact pass null as a value using curl, so this was a situation we needed to allow in the UI.
Before:
before.mov
After:
fixed.mov