Skip to content
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

Add note about escaping % #218

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add note about escaping % #218

wants to merge 2 commits into from

Conversation

tomayac
Copy link

@tomayac tomayac commented Dec 6, 2022

It looks like a bit of an interop issue:

Chrome:

Screenshot 2022-12-06 at 12 01 42

Safari:

Screenshot 2022-12-06 at 12 01 15

Firefox:

Screenshot 2022-12-06 at 12 01 26


Preview | Diff

@terinjokes
Copy link
Collaborator

terinjokes commented Dec 6, 2022

This makes sense to me, but curious if there's comments from Firefox or Safari.

@photopea
Copy link

photopea commented Dec 6, 2022

I have been using the web console to "print an object" and copy-paste it from a web console into a file since 2010 (for debugging, comparing two versions of data), and it is suddenly broken.

console.log(JSON.stringify( {a:"%%Hi",b:"%Hey"} ));
=>  {"a":"%Hi","b":"%Hey"}

@nchevobbe
Copy link

If I'm reading the spec:

If args’s size is 1, return args.

So the screenshot in #218 (comment) for Chrome seems to be wrong for me. Did Chrome behavior changed recently ?

That being said, if you test with multiple parameters, as expected when using specifiers console.log("%%s", "hello"), Chrome, Firefox and Safari do print %s hello.

So, the change here makes sense to me, but I guess a bug should be filed on Chrome for console.log("%%")

@terinjokes
Copy link
Collaborator

@nchevobbe It shouldn't even get there, it fails the earlier check in Logger. With no rest parameters, it should call straight into Printer, without calling into Formatter. That makes the screenshot incorrect, and following that would also fix @photopea's example.

That said, once properly in Formatter, I think escaping % still makes sense.

@HolgerJeromin
Copy link

I found a similar issue:

console.error( 'message: %s', '%hello%%world%');

Chrome:

message: %hello%world%

Firefox:

message: %hello%%world%

@tomayac
Copy link
Author

tomayac commented Mar 22, 2023

Was reminded of this Issue and wondering what to make of it. Do we agree it's somewhat of an interop issue?

@domfarolino
Copy link
Member

I think so, yes. I guess this PR just needs the appropriate changes to Formatter() (beyond the informative table entry)? Also @terinjokes's comment is interesting about Formatter() not even being invoked for the case in the OP at all, which seems important.

@tomayac
Copy link
Author

tomayac commented Mar 22, 2023

I think so, yes.

Great.

I guess this PR just needs the appropriate changes to Formatter() (beyond the informative table entry)? Also @terinjokes's comment is interesting about Formatter() not even being invoked for the case in the OP at all, which seems important.

This is out of my comfort zone. Edits by maintainers are allowed, so please feel free to pile onto this PR with more changes.

@domfarolino
Copy link
Member

I'd appreciate a first stab at it if possible, as I probably won't have time myself to drive the changes for a little while. But the algorithms you'd need to touch aren't too complicated I promise!

@photopea
Copy link

photopea commented Mar 28, 2023

console.log() with a single parameter should print exactly the string which it received.

When I have an object in JS, I want to save it into a JSON file. I do

 console.log(JSON.stringify(obj)); 

And when I copy-paste it from a console to a text file, I get a different object.

@tomayac
Copy link
Author

tomayac commented Mar 28, 2023

So looking back at the original post #218 (comment), which of the two behaviors is considered correct?

@terinjokes
Copy link
Collaborator

As noted in #218 (comment), the current specification is to not intepret % if there are no rest parameters. IIRC, this case was put into Logger to support logging arbitrary strings in a backwards compatible manner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants