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

Benchmark code fixed -- Port & nest() were not working #51

Merged
merged 4 commits into from
Feb 9, 2020

Conversation

JacobMGEvans
Copy link
Contributor

@JacobMGEvans JacobMGEvans commented Feb 3, 2020

  • wrk needs to be installed locally or alternatively it can be installed into dev dependencies.
  • .next() seems to longer be part of the API, I used .middleware() not sure of tests similar but it worked as a stopgap for completing the tests.
  • Port in the shell file didn't seem to be passing its variable into Node process environment, which server.js was using to listen on, hardcoding the port in both files solved the issue.

related to #52

@niftylettuce
Copy link
Contributor

Did you want to put wrk in dev deps and then this would be ok to merge?

@JacobMGEvans
Copy link
Contributor Author

I was up in the air, I had to brew install wrk for this to operate properly. Weirdly .nest() was giving me an issue though I saw that you had fixed it.

JacobMGEvans and others added 3 commits February 6, 2020 21:18
- New config will not require dev to install wrk with brew or global NPM
- scripts will be improved for perfing
@JacobMGEvans
Copy link
Contributor Author

JacobMGEvans commented Feb 7, 2020

I added the wrk to dev dependencies and will work on having the scripts utilize that later.

Eventually, I want to improve the perfing tooling/code and make it more usable maybe even integrate something like a perf/health check into Travis or something.

I think those can be done in other PR's though.

@niftylettuce
Copy link
Contributor

lmk if this is ready?

@JacobMGEvans
Copy link
Contributor Author

Ready.

@niftylettuce niftylettuce merged commit 61aa1e5 into koajs:master Feb 9, 2020
@niftylettuce
Copy link
Contributor

@JacobMGEvans Could we use get-port as an alternative to hardcoded 3333? https://github.com/sindresorhus/get-port

@JacobMGEvans
Copy link
Contributor Author

@JacobMGEvans Could we use get-port as an alternative to hardcoded 3333? https://github.com/sindresorhus/get-port

@niftylettuce
Could potentially use .env https://github.com/motdotla/dotenv

which would also allow for more environment variables to be dynamic and with hardcoded fallbacks.

JacobMGEvans added a commit to JacobMGEvans/router that referenced this pull request Mar 14, 2020
* benchmark code fixed -- Port & nest() were not working

* Added work to DevDepends and will change scripts
- New config will not require dev to install wrk with brew or global NPM
- scripts will be improved for perfing
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.

2 participants