-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
js/console: Marshal by default native JS objects #2375
Conversation
@codebien does this still happen with your code #644 (comment) |
js/console_test.go
Outdated
@@ -134,7 +134,8 @@ func TestConsole(t *testing.T) { | |||
`"string"`: {Message: "string", Data: logrus.Fields{"source": "console"}}, | |||
`"string","a","b"`: {Message: "string a b", Data: logrus.Fields{"source": "console"}}, | |||
`"string",1,2`: {Message: "string 1 2", Data: logrus.Fields{"source": "console"}}, | |||
`{}`: {Message: "[object Object]", Data: logrus.Fields{"source": "console"}}, | |||
`{}`: {Message: "{}", Data: logrus.Fields{"source": "console"}}, |
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.
I have tried the branch locally, and I think I've spotted some missing supported cases? Namely, it seems that as of this PR's state, JSON arrays are not properly supported, nor are arrays with mixed type values. Moreover, console.log
involving formatting with the backtick notation doesn't work as I anticipated either: console.log(\
{"abc": 123}`), still prints
[object Object]`.
It might not be in scope for this PR, but I wanted to double-check with you to make sure these points don't go unnoticed 👀 🙇🏻
Here's a little test I wrote locally to experiment with it
export default function() {
let emptyObjectBody = JSON.parse(`{}`);
console.log(`Want: ${JSON.stringify(emptyObjectBody)}`);
// console.log(`Got: ${emptyObjectBody}`);
console.log(emptyObjectBody);
let objectBody = JSON.parse(`{ "abc": 123, "easy as": "do re mi" }`);
console.log(`Want: ${JSON.stringify(objectBody)}`);
// console.log(`Got: ${objectBody}`);
console.log(objectBody);
let emptyArrayBody = JSON.parse(`[]`);
console.log(`Want: ${JSON.stringify(emptyArrayBody)}`);
// console.log(`Got: ${emptyArrayBody}`);
console.log(emptyArrayBody);
let arrayBody = JSON.parse(`[1, 2, 3]`);
console.log(`Want: ${JSON.stringify(arrayBody)}`);
// console.log(`Got: ${arrayBody}`);
console.log(arrayBody);
let mixedArrayBody = JSON.parse(`["abc", 123, {"foo": "bar"}, [1, 2, 3]]`);
console.log(`Want: ${JSON.stringify(mixedArrayBody)}`);
// console.log(`Got: ${mixedArrayBody}`);
console.log(mixedArrayBody);
}
outputing this:
INFO[0000] Want: {} source=console
INFO[0000] {} source=console
INFO[0000] Want: {"abc":123,"easy as":"do re mi"} source=console
INFO[0000] {"abc":123,"easy as":"do re mi"} source=console
INFO[0000] Want: [] source=console
INFO[0000] source=console
INFO[0000] Want: [1,2,3] source=console
INFO[0000] 1,2,3 source=console
INFO[0000] Want: ["abc",123,{"foo":"bar"},[1,2,3]] source=console
INFO[0000] abc,123,[object Object],1,2,3 source=console
As a result, I would probably add tests cases covering these scenarios 🙇🏻 What do you think?
3c02d50
to
a8f8a1a
Compare
I applied some changes, @oleiade you're cases should be supported now. I dropped the msg goja.Value, args ...goja.Value)
@mstoykov Do you mean the case of a circular object printed as |
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.
👍 LGTM left only one non-blocking comment
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.
🎉 👍🏻 🌮
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.
LGTM.
Can you please explain how this is breaking change in the PR description. As well as point out how to get the previously reported values when possible.
2bb602f
a8f8a1a
to
2bb602f
Compare
@mstoykov @oleiade @olegbespalov I updated the description with changes and added a check in case of an empty argument list (e.g |
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.
👍🏻
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.
👍
js/console_test.go
Outdated
if tt.expected == "" { | ||
assert.Nil(t, entry, "no logs expected") | ||
return | ||
} |
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.
It likely should just log an empty string (?) this is what firefox does and IMO will be less confusing 🤔
61496d9
to
bb944f9
Compare
The output after the latest change
|
bb944f9
to
8275d29
Compare
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.
💪
Closes #1146
js/console
methods likeconsole.log
andconsole.info
, in the case a JavaScript Object (or an Array) is passed as an argument then it is converted into a string encoded as a JSON object.Objects
Arrays
The previous way can be restored by directly parsing the object to a string using the
String
function.