Add ability to uglify generated code #36
Conversation
|
|
||
| const fastSafeStringify = require('fast-safe-stringify') | ||
|
|
||
| var uglify = null |
There was a problem hiding this comment.
Seems inconsistent between let and var in the code base. Which do you want?
There was a problem hiding this comment.
var is better since is a little faster than let, thanks! :)
bench.js
Outdated
| const stringifyString = require('.')({ type: 'string' }) | ||
| const stringify = require('.')(schema, { uglify: true }) | ||
| const stringifyArray = require('.')(arraySchema, { uglify: true }) | ||
| const stringifyString = require('.')({ type: 'string', uglify: true }) |
There was a problem hiding this comment.
Can you include benchmarks for both uglified and non-uglified versions?
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
LGTM with a nit.
Can you also update the benchmarks?
|
This is also conflicting for the other two PRs, can you rebase as well? |
|
@delvedor I think we can bump v1.0.0 when this lands, it has been pretty stable API-wise for the last releases. |
|
Rebased and separate benchmark added plus updated readme. |
| JSON.stringify obj x 1,774,593 ops/sec ±1.07% (90 runs sampled) | ||
| fast-json-stringify obj x 4,976,369 ops/sec ±1.00% (89 runs sampled) | ||
| JSON.stringify array x 3,288 ops/sec ±5.18% (82 runs sampled) | ||
| fast-json-stringify array x 1,813 ops/sec ±10.21% (71 runs sampled) |
There was a problem hiding this comment.
Yeah, we must definitely fix that issue :S
| "standard": "^10.0.0", | ||
| "tap": "^10.3.0" | ||
| "tap": "^10.3.0", | ||
| "uglify-es": "^3.0.9" |
There was a problem hiding this comment.
We need the es variant to support const in the code, btw
|
@SimenB would you like to help maintaining this library and join our team? |
|
@mcollina I'd love to join up! 🙂 |
|
I've sent you an invitation! |
3 participants
@mcollina Something like this you had in mind?