fix: input escape mapping#311
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/index.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/pathstringifier.test.ts:11
- The expected output for the input 'test\a.b' should be ['test\a', 'b'] based on the escaping logic.
['test\\a.b', ['test\a', 'b']]
gibson042
left a comment
There was a problem hiding this comment.
Oh, I didn't realize you'd be willing to embed a data version—but since you are, 👍.
I do still think it would be valuable to require that every property in meta always finds a json node, though, because there are quite possibly current serializations of relatively simple objects that are silently broken (e.g., { "a.0": /regex that becomes a string/ }, which you'll note does not have any backslashes at all).
| if (!legacyPaths) { | ||
| const isEscapedBackslash = char === '\\' && string.charAt(i + 1) === '\\'; | ||
| if (isEscapedBackslash) { | ||
| segment += '\\'; | ||
| i++; | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
This correctly handles escaped backslashes, but does not implement special behavior for backslashes that are in final position (e.g., "foo.bar.baz\\") or followed by something other than a dot or backslash (e.g., "foo.bar.b\\az" or "foo.bar.b\\\n"). I strongly recommend rejecting such input rather than treating it as synonymous with e.g. "foo.bar.baz\\\\"/"foo.bar.b\\\\az"/"foo.bar.b\\\\\n" (respectively).
| if (!legacyPaths) { | |
| const isEscapedBackslash = char === '\\' && string.charAt(i + 1) === '\\'; | |
| if (isEscapedBackslash) { | |
| segment += '\\'; | |
| i++; | |
| continue; | |
| } | |
| } | |
| if (!legacyPaths && char === '\\') { | |
| const escaped = string.charAt(i + 1); | |
| if (escaped === '\\') { | |
| segment += '\\'; | |
| i++; | |
| continue; | |
| } else if (escaped !== '.') { | |
| throw Error('invalid path'); | |
| } | |
| } |
|
Is there any schedule to publish this? Thank you very much |
|
Hello @Skn0tt @flybayer, thanks for implementing this amazing fix. It works great for my use case (deserializing dot string property names combined with custom classes). I see the pr is already merged into main. Would it be possible to publish a new npm version of superjson soon with this fix included? |
|
cc @flybayer |
Closes #310
There's a bug in the path escaping logic where meta was applied to the wrong values. This PR fixes the escape bug. It also introduces a version tag to toggle the path escaping when deserializing. This ensures backwards compatibility for SuperJSON results that were created on a version with the bug.