Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds anyOf support #50

Merged
merged 3 commits into from
Oct 11, 2017
Merged

Adds anyOf support #50

merged 3 commits into from
Oct 11, 2017

Conversation

JbIPS
Copy link
Contributor

@JbIPS JbIPS commented Sep 27, 2017

To implement #49 issue

@JbIPS JbIPS mentioned this pull request Sep 28, 2017
Closed
@JbIPS JbIPS changed the title Adds failing AnyOf testcase Adds anyOf support Oct 9, 2017
@mcollina
Copy link
Member

mcollina commented Oct 9, 2017

CI is failing on node 4.

@JbIPS
Copy link
Contributor Author

JbIPS commented Oct 9, 2017

Damn, forgot rest params are not supported before 6.x. I'll find a workaround

@JbIPS
Copy link
Contributor Author

JbIPS commented Oct 9, 2017

All green \o/

mcollina
mcollina reviewed Oct 10, 2017
View reviewed changes
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.

Can you check in the benchmarks have been impacted by the change? (run on them on master vs this branch)

index.js Outdated
}
if (dependencies.length > 0) {
dependenciesName.push(code)
return (Function.apply(null, dependenciesName).apply(null, dependencies))
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove the if (dependencies.length > 0)  check.

Removes useless check
dd56c80 
* Removes a check on dependencies list
* Adds a benchmark script
@JbIPS
Copy link
Contributor Author

JbIPS commented Oct 10, 2017

With Node 8.4.0
Fork

JSON.stringify array x 3,736 ops/sec ±0.73% (93 runs sampled)
fast-json-stringify array x 4,817 ops/sec ±0.69% (94 runs sampled)
fast-json-stringify-uglified array x 4,913 ops/sec ±0.76% (94 runs sampled)
JSON.stringify long string x 10,962 ops/sec ±0.49% (92 runs sampled)
fast-json-stringify long string x 10,968 ops/sec ±0.67% (95 runs sampled)
fast-json-stringify-uglified long string x 10,851 ops/sec ±1.00% (91 runs sampled)
JSON.stringify short string x 4,380,260 ops/sec ±0.71% (92 runs sampled)
fast-json-stringify short string x 9,730,481 ops/sec ±0.29% (93 runs sampled)
fast-json-stringify-uglified short string x 9,664,561 ops/sec ±0.87% (93 runs sampled)
JSON.stringify obj x 1,831,760 ops/sec ±0.38% (94 runs sampled)
fast-json-stringify obj x 4,448,195 ops/sec ±0.90% (94 runs sampled)
fast-json-stringify-uglified obj x 4,557,107 ops/sec ±0.24% (92 runs sampled)

Origin

JSON.stringify array x 3,827 ops/sec ±0.49% (93 runs sampled)
fast-json-stringify array x 4,660 ops/sec ±0.48% (91 runs sampled)
fast-json-stringify-uglified array x 4,733 ops/sec ±0.82% (92 runs sampled)
JSON.stringify long string x 10,939 ops/sec ±0.25% (93 runs sampled)
fast-json-stringify long string x 10,922 ops/sec ±0.61% (95 runs sampled)
fast-json-stringify-uglified long string x 10,972 ops/sec ±0.42% (92 runs sampled)
JSON.stringify short string x 4,306,996 ops/sec ±0.79% (95 runs sampled)
fast-json-stringify short string x 9,568,480 ops/sec ±0.71% (92 runs sampled)
fast-json-stringify-uglified short string x 9,758,963 ops/sec ±0.81% (92 runs sampled)
JSON.stringify obj x 1,804,430 ops/sec ±1.13% (95 runs sampled)
fast-json-stringify obj x 4,634,735 ops/sec ±0.79% (94 runs sampled)
fast-json-stringify-uglified obj x 4,712,696 ops/sec ±0.44% (95 runs sampled)

mcollina
mcollina approved these changes Oct 10, 2017
View reviewed changes
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

delvedor
delvedor approved these changes Oct 10, 2017
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 85f1e0e into fastify:master Oct 11, 2017
@mcollina
Copy link
Member

Can you send another PR with the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@delvedor delvedor delvedor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JbIPS @mcollina @delvedor