-
-
Notifications
You must be signed in to change notification settings - Fork 210
Performance improvment: Add format "raw" for "string" type (skip escaping) #685
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
Changes from all commits
6db646c
749b652
a46bf0b
9b53004
56e0331
5d22777
ce2caa5
5f27e08
fe7cd4a
60f7193
58768cc
b380401
55bddff
4f30811
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ module.exports = class Serializer { | |
throw new Error(`The value "${date}" cannot be converted to a time.`) | ||
} | ||
|
||
asString (str) { | ||
asString (str, format) { | ||
if (typeof str !== 'string') { | ||
if (str === null) { | ||
return '""' | ||
|
@@ -113,7 +113,9 @@ module.exports = class Serializer { | |
} | ||
} | ||
|
||
if (str.length < 42) { | ||
if (format === 'raw') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would create a different serializer method instead of adding the check here, because you know the if condition at the compile time and there is no need to to it in a serialization time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mhh eg. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use a different name than @mcollina WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for unsafe ... |
||
return '"' + str + '"' | ||
} else if (str.length < 42) { | ||
return this.asStringSmall(str) | ||
} else if (STR_ESCAPE.test(str) === false) { | ||
return '"' + str + '"' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,22 @@ const t = require('tap') | |
const test = t.test | ||
const build = require('..') | ||
|
||
test('serialize short string raw', (t) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add some test cases where it doesn't escape chars |
||
t.plan(2) | ||
|
||
const schema = { | ||
type: 'string', | ||
format: 'raw' | ||
} | ||
|
||
const input = 'abcd' | ||
const stringify = build(schema) | ||
const output = stringify(input) | ||
|
||
t.equal(output, '"abcd"') | ||
t.equal(JSON.parse(output), input) | ||
}) | ||
|
||
test('serialize short string', (t) => { | ||
t.plan(2) | ||
|
||
|
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.
Why pass null? It’s type is object while the parameter type is string — to me that gives me the impression that an object argument is expected there
What’s wrong with leaving it as undefined?