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

[now-static-build] Fix dev server port detection #2879

Merged

Conversation

callain
Copy link
Contributor

@callain callain commented Aug 27, 2019

Really check if the port is reachable instead of reading the console data.
For example CRA does not show the devPort in the console if there is a warning in the code, meaning now dev fail to start.

Fixes #2856

Copy link
Contributor

@leo leo left a comment

Choose a reason for hiding this comment

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

If @AndyBitz and @TooTallNate have no objections, this looks really good!

@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #2879 into canary will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           canary    #2879   +/-   ##
=======================================
  Coverage   13.38%   13.38%           
=======================================
  Files         268      268           
  Lines       10342    10342           
  Branches     1218     1218           
=======================================
  Hits         1384     1384           
  Misses       8896     8896           
  Partials       62       62

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 e1a770a...7385cff. Read the comment docs.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!!

What do you think @TooTallNate ?

@styfle styfle changed the title [now-static-build] Fix dev server detection [now-static-build] Fix dev server port detection Aug 30, 2019
@callain
Copy link
Contributor Author

callain commented Sep 6, 2019

Up :)

Copy link
Member

@TooTallNate TooTallNate left a comment

Choose a reason for hiding this comment

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

Tested and it works nicely. There's a few "tweaks" I'd still like to make, but I will do that in a separate PR. Thanks @callain!

@kodiakhq kodiakhq bot merged commit 96b3c1e into vercel:canary Sep 6, 2019
@callain
Copy link
Contributor Author

callain commented Sep 8, 2019

Thanks ! I'm sure it will help a lot of users 😃

styfle pushed a commit that referenced this pull request Sep 17, 2019
* [now-static-build] Fix dev server detection

* Code review

* Remove unused dependency

* Fix the checking by really waiting until the port is reachable
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.

[now dev] failed to start when there is warnings in CRA app
5 participants