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

feat: retries if default port is occupied #64

Merged
merged 4 commits into from
Aug 18, 2020

Conversation

pantharshit00
Copy link
Member

@pantharshit00 pantharshit00 commented Aug 15, 2020

Yay, first PR to abell. More to come :)

What type of PR is this?

  • Feature
  • Bug Fix
  • Refactor
  • Documentation Update

Description

Closes #61

This PR recursively find the next available port if default port is used until 3 levels. It doesn't run this for non-default ports as user has explicitly specified the port there.

Screenshots:

Port 5000 is avaliable

image

Port 5000 taken

image

Port 5000, 5001 and 5002 taken

image

Port 5000, 5001, 5002 and 5003 taken(case which exceed the max retries)

image

Open questions:

  • Should we prompt the user instead of doing this by default. Creating a prompt will require additional effort as we don't have one already in the project.

Added unit tests?

  • No, I want help in writing tests.
  • No, I want someone else to write tests.
  • No, The addition does not need tests.
  • Yes

(I will add them tomorrow, too sleepy right now)

[optional] Random GIF to +1 someone's day

Here is my dog's picture(name is Arrow :) )
image

@pantharshit00 pantharshit00 marked this pull request as draft August 15, 2020 22:05
@pantharshit00
Copy link
Member Author

pantharshit00 commented Aug 15, 2020

I will add tests soon then this will be ready for merge

@saurabhdaware
Copy link
Member

Should we prompt the user instead of doing this by default. Creating a prompt will require additional effort as we don't have one already in the project.

No, I think this way is much better than having a prompt.

@saurabhdaware
Copy link
Member

P.S. Arrow is suchaa cutieeeee!!! When you all celebrate his birthday or any other day for him, is it called Arrow Function :p ?

@pantharshit00
Copy link
Member Author

pantharshit00 commented Aug 16, 2020

Re P.S. : haha, his birthday is on 7th July

My brother actually named him from the TV series Arrows. First time I have linked him with js arrow fns because of you 😅.

@pantharshit00
Copy link
Member Author

Hold up, we have no tests for dev server 😅

@saurabhdaware
Copy link
Member

oh yes lol forgot. Here #35

@saurabhdaware
Copy link
Member

I will test this code manually tomorrow and we can write tests for dev-server after that. Any idea on how we can test it though?

@pantharshit00
Copy link
Member Author

@saurabhdaware I will add test for the whole deal, don't worry now. This one is on me now

@saurabhdaware
Copy link
Member

Woah! That will be huge help 🎉

@pantharshit00
Copy link
Member Author

Ok, this is ready for review now. I have added tests for http server and the retry logic.

@@ -97,7 +101,7 @@ describe("utils/abell-fs.js - Abell's file system", () => {
});

after(() => {
process.chdir('../../../..');
process.chdir(__dirname);
Copy link
Member Author

Choose a reason for hiding this comment

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

These path changes were very unreliable so I had to refactor them

@pantharshit00
Copy link
Member Author

Opened #72 for adding tests for wss

Copy link
Member

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

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

THIS. IS. AMAZINGGGGGGGG 🎉 I was soo complicating things lol using fetch is such a good idea!

@saurabhdaware saurabhdaware merged commit 63bc8e2 into abelljs:main Aug 18, 2020
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.

Dev server: listen on next available port if port 5000 is not free
2 participants