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.

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

Refactor RoutePrefix to be a string #615

Merged
merged 1 commit into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@nwoltman
Copy link
Member

commented Jan 4, 2018

It used to be a class with a single prefix property that didn't have any extra functionality.
The prefix property as a string is all that's needed.

benchmark

master

[1] Stat         Avg     Stdev  Max
[1] Latency (ms) 3.11    11.01  360
[1] Req/Sec      31406.4 6013.7 35647
[1] Bytes/Sec    4.73 MB 897 kB 5.51 MB
[1]
[1] 157k requests in 5s, 23.4 MB read
...
[1] Stat         Avg     Stdev   Max
[1] Latency (ms) 3.01    10.83   371
[1] Req/Sec      32379.2 6412.03 37567
[1] Bytes/Sec    4.84 MB 964 kB  5.77 MB
[1]
[1] 162k requests in 5s, 24.1 MB read

this PR

[1] Stat         Avg    Stdev  Max
[1] Latency (ms) 3.16   11.16  371
[1] Req/Sec      30880  6228   35999
[1] Bytes/Sec    4.6 MB 912 kB 5.51 MB
[1]
[1] 154k requests in 5s, 23 MB read
...
[1] Stat         Avg     Stdev   Max
[1] Latency (ms) 3.01    10.7    373
[1] Req/Sec      32462.4 6622.57 36767
[1] Bytes/Sec    4.81 MB 1 MB    5.51 MB
[1]
[1] 162k requests in 5s, 24.2 MB read

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct
@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

I wonder if the object was there due to V8 classes.

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2018

Something about keeping the same shape of the fastify instance perhaps?

@allevo

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Could you explain why of this PR?

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2018

Makes the code easier to understand and maintain.

@allevo
Copy link
Member

left a comment

I'm always 👍 when some code is removed!

}
return R
return instancePrefix + pluginPrefix

This comment has been minimized.

Copy link
@allevo

allevo Jan 4, 2018

Member

What happen with buildRoutePrefix('/foo/', 'bar)? the returned string is /foo//bar

This comment has been minimized.

Copy link
@nwoltman

nwoltman Jan 4, 2018

Author Member

Yes. I think that's what the discussion in #584 is about.

@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Something about keeping the same shape of the fastify instance perhaps?

Exactly. I'd like to hear what @mcollina and/or @delvedor have to say before I add a LGTM.

@delvedor
Copy link
Member

left a comment

LGTM

The route prefix was a class because originally we have prepared it to add methods in the future, but we never did, because the feature was already ok :)

I'm really ok with this change, we just need to be sure that the encapsulation model will continue to work as expected.
Maybe you can add some additional test here, to verify thats this will continue to work with two or more register.

@jsumners
Copy link
Member

left a comment

LGTM with extra testing as suggested

@delvedor delvedor added the enhancement label Jan 4, 2018

Refactor RoutePrefix to be a string
It used to be a class with a single `prefix` property that didn't have any extra functionality.
The prefix property as a string is all that's needed.

@nwoltman nwoltman force-pushed the nwoltman:route-prefix branch from dc9d30b to b4c0a17 Jan 5, 2018

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2018

Thanks for the clarification @delvedor :)
I added a new test with multiple register calls.

@mcollina mcollina merged commit 0ec101c into fastify:master Jan 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mcollina

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

Thanks!

@nwoltman nwoltman deleted the nwoltman:route-prefix branch Jan 5, 2018

@delvedor delvedor referenced this pull request Jan 6, 2018

Merged

Updated route prefix test #626

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.