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

Finished up work started in PR (#263) auto incrementing the port if it is already in use #278

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alallier
Copy link
Owner

@alallier alallier commented Nov 19, 2020

Finished up work started by @al5ina5 in PR (#263) auto incrementing the port if it is already in use

This adds the following ontop of the work already started by @al5ina5:

  • Added new tests
  • Added the port to reload's return object so the calling program knows what port reload ending up using if the port auto increments
  • Added autoIncrementPort optional param to realod to turn off auto port incrementing. Default true
  • Tweaked the code to work correctly, like handling the promise return correctly
  • Small tweaks to how the generated client side code replace works. Removed a branch that wasn't hitting in coverage and made it more readable
  • Updated README with API changes

Closes #256

Branched off work from PR #263

@alallier alallier changed the title Finished up work started by @al5ina5 in PR (#263) auto incrementing the port if it is already in use Finished up work started in PR (#263) auto incrementing the port if it is already in use Nov 19, 2020
@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #278 (ff58c31) into master (b3c4e8c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #278   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          131       144   +13     
=========================================
+ Hits           131       144   +13     
Impacted Files Coverage Δ
lib/reload.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0271a6...ff58c31. Read the comment docs.

@alallier alallier mentioned this pull request Nov 19, 2020
lib/reload.js Show resolved Hide resolved
lib/reload.js Outdated Show resolved Hide resolved
@al5ina5
Copy link

al5ina5 commented Nov 19, 2020

Hey, this is awesome to see. 👍

Thanks for tagging me. Hope I didn’t give you too much trouble! 🤞🏼

@alallier
Copy link
Owner Author

alallier commented Nov 19, 2020

@al5ina5 no problem at all you got the ball rolling. You should be a first time reload contributor soon when this gets merged!

@alallier
Copy link
Owner Author

Per #256 we should still reject if the auto increment option is turned off and there is a port conflict

… the port if it is already in use. This commit adds the following to the existing work:

* Added new tests
* Added the port to reload's return object so the calling program knows what port reload ending up using if the port auto increments
* Added autoIncrementPort optional param to realod to turn off auto port incrementing. Default `true`
* Tweaked the code to work correctly, like handling the promise return correctly
* Small tweaks to how the generated client side code replace works. Removed a branch that wasn't hitting in coverage and made it more readable
* Updated README with API changes
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.

Reject with error on http server error
2 participants