Skip to content

Conversation

@sinclairzx81
Copy link
Contributor

This PR fixes the linting warnings across the codebase to resolve issue #281. It replaces var for let and const where appropriate.

Note: The majority of the replacements were fairly straightforward, however I did spot a possibly undefined condition on line 621 with respect to the variable walk. You may wish to double check the additional ref check I've added on this code path.

Checklist

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@Eomm
Copy link
Member

Eomm commented Jan 7, 2021

Did you run the banchmark too?
Could you share the results?

@mcollina
Copy link
Member

mcollina commented Jan 7, 2021

You should not replace for (var i with for (let i, it's slower in most cases in supported node versions.

@sinclairzx81
Copy link
Contributor Author

sinclairzx81 commented Jan 8, 2021

@Eomm Hi, see below for comparative benchmarks using let | const vs var currently on the master branch. Results do vary a little from test to test, but seem about on par.

test machine

cpu  AMD Ryzen 7 3700X 8-Core Processor
ram  32GB 
node v12.18.2

using let | const

FJS creation x 56,896 ops/sec ±0.32% (94 runs sampled)
CJS creation x 160,658 ops/sec ±0.29% (94 runs sampled)

JSON.stringify array x 3,314 ops/sec ±0.31% (96 runs sampled)
fast-json-stringify array x 8,388 ops/sec ±0.56% (91 runs sampled)

compile-json-stringify array x 7,319 ops/sec ±0.50% (92 runs sampled)
JSON.stringify long string x 10,085 ops/sec ±0.10% (97 runs sampled)

fast-json-stringify long string x 10,087 ops/sec ±0.15% (96 runs sampled)
compile-json-stringify long string x 10,069 ops/sec ±0.14% (98 runs sampled)

JSON.stringify short string x 8,644,967 ops/sec ±0.32% (91 runs sampled)
fast-json-stringify short string x 46,317,880 ops/sec ±0.41% (96 runs sampled)

compile-json-stringify short string x 38,401,762 ops/sec ±0.88% (97 runs sampled)
JSON.stringify obj x 1,824,375 ops/sec ±0.19% (93 runs sampled)

fast-json-stringify obj x 16,346,021 ops/sec ±0.43% (91 runs sampled)
compile-json-stringify obj x 22,268,550 ops/sec ±0.80% (97 runs sampled)

var (master)

FJS creation x 56,203 ops/sec ±0.58% (94 runs sampled)
CJS creation x 161,716 ops/sec ±0.33% (92 runs sampled)

JSON.stringify array x 3,310 ops/sec ±0.24% (95 runs sampled)
fast-json-stringify array x 8,026 ops/sec ±2.21% (87 runs sampled)

compile-json-stringify array x 8,131 ops/sec ±0.35% (94 runs sampled)
JSON.stringify long string x 10,076 ops/sec ±0.16% (97 runs sampled)

fast-json-stringify long string x 10,082 ops/sec ±0.19% (95 runs sampled)
compile-json-stringify long string x 10,067 ops/sec ±0.19% (98 runs sampled)

JSON.stringify short string x 8,695,700 ops/sec ±0.35% (95 runs sampled)
fast-json-stringify short string x 47,173,708 ops/sec ±0.17% (95 runs sampled)

compile-json-stringify short string x 38,136,956 ops/sec ±0.42% (98 runs sampled)
JSON.stringify obj x 1,833,705 ops/sec ±0.14% (96 runs sampled)

fast-json-stringify obj x 16,740,274 ops/sec ±1.56% (92 runs sampled)
compile-json-stringify obj x 22,381,926 ops/sec ±0.76% (96 runs sampled)

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

Miss this one

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm (I do not see regressions in the benchmarks)

@mcollina mcollina merged commit 5682018 into fastify:master Jan 8, 2021
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.

5 participants