Skip to content

Conversation

cesco69
Copy link
Contributor

@cesco69 cesco69 commented Sep 23, 2025

isWellFormed don't works like the regex /[\u0000-\u001f\u0022\u005c\ud800-\udfff]/

This commit must be reverted
7f48cb6

This PR #774 just pass the test, because no test match the condition >40 chars <5000

I have added a test (serialize medium string) that enter in this condition

# Subtest: serialize short string
ok 362 - serialize short string

# Subtest: serialize medium string
not ok 363 - serialize medium string

# Subtest: serialize long string
ok 364 - serialize long string

The regex [\u0000-\u001f\u0022\u005c\ud800-\udfff]/ check for:

  1. ASCII (U+0000–U+001F)
  2. Char "
  3. Char \
  4. UTF-16 (\uD800–\uDFFF)

isWellFormed() only check case 4

These are case need escape but isWellFormed says are good!

'"'.isWellFormed()
true

'\\'.isWellFormed()
true

'\n'.isWellFormed()
true

Signed-off-by: francesco <[email protected]>
@cesco69 cesco69 changed the title test: isWellFormed test: isWellFormed (PR 774) break the lib Sep 23, 2025
@cesco69 cesco69 mentioned this pull request Sep 23, 2025
@nigrosimone
Copy link
Contributor

isWellFormed return true for string need escaping!

const str = 'abc\ndef';
str.isWellFormed()
true
JSON.stringify(str)
'"abc\\ndef"'

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be true

@gurgunday
Copy link
Member

@Uzlopak wdyt?

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 27, 2025

@gurgunday

Well, @cesco69 ist right.

According to MDN isWellFormed just gives true if the string does not contain lone surrogates. Thats what @cesco69 means with only case 4 is covered.

So to fix this we have two options:

  1. revert to the old regex,
  2. use isWellFormed in combination with a shorter regex, like this pseudocode
const ASCII_ESCAPE = /[\u0000-\u001f\u0022\u005c]/u

if (str.isWellFormed() && ASCII_ESCAPE.test(str) === false) {
  // we need a bigger boat
}

We should benchmark the options.

@nigrosimone nigrosimone mentioned this pull request Sep 27, 2025
4 tasks
@cesco69
Copy link
Contributor Author

cesco69 commented Sep 29, 2025

see #792

@cesco69 cesco69 closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants