Allow non alpha keys

[SimenB]

I tried this:

diff --git i/index.js w/index.js
index ff80f46..923d310 100644
--- i/index.js
+++ w/index.js
@@ -306,7 +306,7 @@ function buildObject (schema, code, name, externalSchema, fullSchema) {
     // Using obj.key !== undefined instead of obj.hasOwnProperty(prop) for perf reasons,
     // see https://github.com/mcollina/fast-json-stringify/pull/3 for discussion.
     code += `
-      if (obj.${key} !== undefined) {
+      if (obj['${key}'] !== undefined) {
         json += '${$asString(key)}:'
       `
 
@@ -392,6 +392,7 @@ function nested (laterCode, name, key, schema, externalSchema, fullSchema) {
   var code = ''
   var funcName
   const type = schema.type
+  const value = `obj['${key.substring(1)}']`
   switch (type) {
     case 'null':
       code += `
@@ -400,36 +401,36 @@ function nested (laterCode, name, key, schema, externalSchema, fullSchema) {
       break
     case 'string':
       code += `
-        json += $asString(obj${key})
+        json += $asString(${value})
       `
       break
     case 'integer':
       code += `
-        json += $asInteger(obj${key})
+        json += $asInteger(${value})
       `
       break
     case 'number':
       code += `
-        json += $asNumber(obj${key})
+        json += $asNumber(${value})
       `
       break
     case 'boolean':
       code += `
-        json += $asBoolean(obj${key})
+        json += $asBoolean(${value})
       `
       break
     case 'object':
       funcName = (name + key).replace(/[-.\[\]]/g, '') // eslint-disable-line
       laterCode = buildObject(schema, laterCode, funcName, externalSchema, fullSchema)
       code += `
-        json += ${funcName}(obj${key})
+        json += ${funcName}(${value})
       `
       break
     case 'array':
       funcName = (name + key).replace(/[-.\[\]]/g, '') // eslint-disable-line
       laterCode = buildArray(schema, laterCode, funcName, externalSchema, fullSchema)
       code += `
-        json += ${funcName}(obj${key})
+        json += ${funcName}(${value})
       `
       break
     default:

The top part worked fine, it failed a bunch of tests with the second one, though

[mcollina]

I think you should have an helper that spits out key.substring(1) if key starts with @. It should work

I think the problem you are encountering is because the same logic works for both object and arrays.

[SimenB]

Wouldn't this be a problem for all keys not being Aa-Zz?

[mcollina]

@SimenB very probably.

[SimenB]

I'm a bit stuck here... Why is key prefixed with . anyways? And is there a perf hit for doing obj['key'] instead of obj.key? I've pushed what I had locally, would love some guidance on how to fix it.

[SimenB]

@mcollina I made it green at least. Not too happy with the implementation though... Ideas?

[mcollina]

I think we should use the dot notation wherever possible. It should be faster.

Is this impacting benchmarks anyhow?

[mcollina]

can you use the same accessor here?

[SimenB]

accessor is in the nested function, not up here.

[SimenB]

Benchmarks (on [email protected]):
Master:

JSON.stringify array x 3,666 ops/sec ±1.84% (81 runs sampled)
fast-json-stringify array x 2,337 ops/sec ±2.16% (83 runs sampled)
JSON.stringify long string x 14,201 ops/sec ±1.55% (80 runs sampled)
fast-json-stringify long string x 13,621 ops/sec ±1.90% (82 runs sampled)
JSON.stringify short string x 5,156,172 ops/sec ±1.87% (83 runs sampled)
fast-json-stringify short string x 12,075,130 ops/sec ±1.79% (81 runs sampled)
JSON.stringify obj x 1,729,733 ops/sec ±1.71% (79 runs sampled)
fast-json-stringify obj x 2,308,956 ops/sec ±2.22% (81 runs sampled)

This branch:

JSON.stringify array x 3,595 ops/sec ±2.19% (80 runs sampled)
fast-json-stringify array x 2,336 ops/sec ±2.11% (81 runs sampled)
JSON.stringify long string x 13,642 ops/sec ±1.65% (84 runs sampled)
fast-json-stringify long string x 13,699 ops/sec ±2.62% (84 runs sampled)
JSON.stringify short string x 4,928,457 ops/sec ±1.86% (78 runs sampled)
fast-json-stringify short string x 11,600,544 ops/sec ±1.71% (84 runs sampled)
JSON.stringify obj x 1,775,965 ops/sec ±2.34% (79 runs sampled)
fast-json-stringify obj x 2,309,468 ops/sec ±2.29% (82 runs sampled)

[SimenB]

It could have been interesting to pass the generated code through a minifier, which would remove the brackets if they're unnecessary (uglilfy does this). Not sure if you want that dep, though...

[SimenB]

I tested it out (because why not). Result

JSON.stringify array x 4,008 ops/sec ±1.64% (87 runs sampled)
fast-json-stringify array x 2,520 ops/sec ±2.12% (85 runs sampled)
JSON.stringify long string x 14,634 ops/sec ±1.31% (88 runs sampled)
fast-json-stringify long string x 14,446 ops/sec ±1.53% (85 runs sampled)
JSON.stringify short string x 5,382,246 ops/sec ±1.83% (82 runs sampled)
fast-json-stringify short string x 12,582,489 ops/sec ±2.26% (86 runs sampled)
JSON.stringify obj x 1,741,939 ops/sec ±1.57% (87 runs sampled)
fast-json-stringify obj x 2,404,493 ops/sec ±2.52% (80 runs sampled)

Diff:

diff --git i/index.js w/index.js
index ff80f46..d51febb 100644
--- i/index.js
+++ w/index.js
@@ -1,6 +1,7 @@
 'use strict'
 
 const fastSafeStringify = require('fast-safe-stringify')
+const uglify = require('uglify-es')
 
 let isLong
 try {
@@ -74,10 +75,16 @@ function build (schema, options) {
     ;
     return ${main}
   `
+
+  const uglified = uglify.minify(code, {parse: {bare_returns: true}})
+
+  if (uglified.error) {
+    throw uglified.error
+  }
   if (schema.additionalProperties === true) {
-    return (new Function('fastSafeStringify', code))(fastSafeStringify)
+    return (new Function('fastSafeStringify', uglified.code))(fastSafeStringify)
   }
-  return (new Function(code))()
+  return (new Function(uglified.code))()
 }
 
 function $asNull () {
diff --git i/package.json w/package.json
index 5ebd8a8..0fb85b9 100644
--- i/package.json
+++ w/package.json
@@ -32,6 +32,7 @@
     "tap": "^10.3.0"
   },
   "dependencies": {
-    "fast-safe-stringify": "^1.1.11"
+    "fast-safe-stringify": "^1.1.11",
+    "uglify-es": "^3.0.9"
   }
 }

[mcollina]

LGTM.

Regarding uglify, I'm definitely +1, as the numbers look nice. I would like to have that but avoid a direct dependency to uglify, as this module is tight and nice. Would you like to send a PR?

[SimenB]

How would we pass it through a minimizer without depending on the minimizer? Try to require it in case it's available?

[mcollina]

you can try requiring it, and if it is there use it. Plus, we can put it under an option as well. Il giorno sab 20 mag 2017 alle 20:43 Simen Bekkhus <[email protected]> ha scritto:

…

How would we pass it through a minimizer without depending on the minimizer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL4zT9T9oZSSRFx2DmOWKevYgWnu1Pks5r7zRjgaJpZM4NgZ7F> .

[SimenB]

Cool, PR sent 😄

Will you be able to merge and release this and #35 by Monday?