Skip to content

Upgrade detect-port - #2126

Closed
Timer wants to merge 1 commit into
react:masterfrom
Timer:detect-port-upgrade
Closed

Timer wants to merge 1 commit into
react:masterfrom
Timer:detect-port-upgrade

Conversation

@Timer

@Timer Timer commented May 11, 2017

Copy link
Copy Markdown
Contributor

Upgrades detect port to resolve #2112.

Test plan: start two servers and expect the duplicate message, do the same with a different HOST.
It works.

I'm not sure about edge cases (like binding to a hostname instead of address, e.g. boop.local).

@gaearon

gaearon commented May 11, 2017

Copy link
Copy Markdown
Contributor

Not working for me.

screen shot 2017-05-11 at 5 25 00 pm

screen shot 2017-05-11 at 5 25 09 pm

@Timer

Timer commented May 11, 2017

Copy link
Copy Markdown
Contributor Author

Weird.. I wonder what differs between our macs.
I'll look at this again in a few.

@gaearon

gaearon commented May 11, 2017

Copy link
Copy Markdown
Contributor

The stack is weird, isn't it? Doesn't seem to be inside the initial promise code.

@gaearon

gaearon commented May 11, 2017

Copy link
Copy Markdown
Contributor

This doesn't work for me:

  // 1. check 0.0.0.0
  listen(port, null, (err, realPort) => {

This does:

  // 1. check 0.0.0.0
  listen(port, '0.0.0.0', (err, realPort) => {

@Timer

Timer commented May 11, 2017

Copy link
Copy Markdown
Contributor Author

Ooo. Maybe the latest release introduced a regression.
I'll be able to check it out in a bit.

To be fair, I ran the first application using an older CRA.

@Timer

Timer commented May 11, 2017 •

Copy link
Copy Markdown
Contributor Author

Yeah, that's because null binds to :: by default (IPv6), which also binds to 0.0.0.0 in a fallback-ish-way.

I asked again for the original request, which is to let us explicitly pass which host we'd like to check.

The testing was my bad, it was because the old CRA version bound to a different address (which let detect-port work with its patch).

@gaearon

gaearon commented May 11, 2017

Copy link
Copy Markdown
Contributor

What's the conclusion? How do we fix?

@Timer

Timer commented May 11, 2017 •

Copy link
Copy Markdown
Contributor Author

We can either revert this behavior entirely (breaking HOST), we can add logic to bind to IPv6 by default (when available, still doesn't fix the underlying issue but most cases), or wait for another release of detect-port.

@Timer

Timer commented May 11, 2017 •

Copy link
Copy Markdown
Contributor Author

We could also just fork detect-port for our own use.

@gaearon

gaearon commented May 12, 2017

Copy link
Copy Markdown
Contributor

I'm okay with temporarily forking.

@Timer Timer closed this May 14, 2017
@Timer
Timer deleted the detect-port-upgrade branch May 14, 2017 18:51
@lock lock Bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Master crashes when opening two apps (and port conflicts)

3 participants