Skip to content

Add note about escaping %#218

Open
tomayac wants to merge 2 commits into
whatwg:mainfrom
tomayac:patch-1
Open

Add note about escaping %#218
tomayac wants to merge 2 commits into
whatwg:mainfrom
tomayac:patch-1

Conversation

@tomayac

@tomayac tomayac commented Dec 6, 2022

Copy link
Copy Markdown

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

terinjokes commented Dec 6, 2022

Copy link
Copy Markdown
Collaborator

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

@photopea

photopea commented Dec 6, 2022

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

I found a similar issue:

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

Chrome:

message: %hello%world%

Firefox:

message: %hello%%world%

@tomayac

tomayac commented Mar 22, 2023

Copy link
Copy Markdown
Author

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
Copy Markdown
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

tomayac commented Mar 22, 2023

Copy link
Copy Markdown
Author

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
Copy Markdown
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

photopea commented Mar 28, 2023

Copy link
Copy Markdown

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

tomayac commented Mar 28, 2023

Copy link
Copy Markdown
Author

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

@terinjokes

Copy link
Copy Markdown
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