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.

Sign up for GitHub

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

Jump to bottom

refactor(package): update internal-ip v1.2.0...3.0.0 (dependencies) #1471

Closed
wants to merge 1 commit into from
Closed

refactor(package): update internal-ip v1.2.0...3.0.0 (dependencies) #1471

wants to merge 1 commit into from

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Aug 25, 2018 •
edited by michael-ciniawsky

internal-ip was brought back down because of #1109, but they've since added a sync method

Type

  • Refactor

Issues

SemVer

  • Patch

@michael-ciniawsky michael-ciniawsky changed the title refactor: bump internal-ip refactor(util/createDomain): use v4.sync() instead of v4() (internal-ip) Aug 25, 2018
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 25, 2018 •
edited

The clearify the main issue in #1109 was simply the fact that internal-ip changed it's API from v1.0.0...2.0.0 making internal.v4 async and now v3.0.0 added a new sync method internal.v4.sync() instead ?

@michael-ciniawsky michael-ciniawsky modified the milestones: 3.1.6, 3.2.0 Aug 25, 2018
@michael-ciniawsky michael-ciniawsky added this to Refactor in Dashboard Aug 25, 2018
michael-ciniawsky
michael-ciniawsky approved these changes Aug 26, 2018
View reviewed changes
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment •
edited

Choose a reason for hiding this comment

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

@wtgtybhertgeghgtwtg Thx 👍 Please rebase :)

@codecov
Copy link

codecov bot commented Aug 26, 2018 •
edited

Codecov Report

Merging #1471 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1471   +/-   ##
=======================================
  Coverage   71.82%   71.82%           
=======================================
  Files          11       11           
  Lines         763      763           
=======================================
  Hits          548      548           
  Misses        215      215
Impacted Files Coverage Δ
lib/utils/createDomain.js 100% <100%> (ø) ⬆️

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 2d0999d...6fa0de2. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title refactor(util/createDomain): use v4.sync() instead of v4() (internal-ip) refactor(package): update internal-ip v1.2.0...3.0.0 (dependencies) Aug 26, 2018
@michael-ciniawsky michael-ciniawsky removed the request for review from alexander-akait August 26, 2018 05:21
@michael-ciniawsky
Copy link
Member 

1) check utility funcitons test createDomain 'localIp':
 Error: done() invoked with non-Error: generated domain http: doesn't match expected http://null:8080
   at /home/travis/build/webpack/webpack-dev-server/node_modules/mocha/lib/runnable.js:380:21
   at Server.server.listen (/home/travis/build/webpack/webpack-dev-server/test/Util.test.js:83:11)
   at Server.returnValue.listeningApp.listen.err (/home/travis/build/webpack/webpack-dev-server/lib/Server.js:78:2095)
   at Object.onceWrapper (events.js:273:13)
   at Server.emit (events.js:182:13)
   at emitListeningNT (net.js:1368:10)
   at process._tickCallback (internal/process/next_tick.js:63:19)

http://null:8080

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

wtgtybhertgeghgtwtg commented Aug 26, 2018 •
edited

It works locally, so I'm guessing it just couldn't get the IP inside Travis and that url.format will just be a protocol if hostname is null, so it won't match http://${internalIp.v4.sync()}:8080.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

To clarify, internal-ip used to give a fallback if it couldn't find the IP, which I'm guessing is why that test worked. What should I do with it?

alexander-akait
alexander-akait reviewed Aug 27, 2018
View reviewed changes
lib/util/createDomain.js Outdated
@@ -8,7 +8,7 @@ module.exports = function createDomain(options, listeningApp) {
const protocol = options.https ? 'https' : 'http';
const appPort = listeningApp ? listeningApp.address().port : 0;
const port = options.socket ? 0 : appPort;
const hostname = options.useLocalIp ? internalIp.v4() : options.host;
const hostname = options.useLocalIp ? internalIp.v4.sync() : options.host;
Copy link
Member

Choose a reason for hiding this comment

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

const hostname = options.useLocalIp ? internalIp.v4.sync() || 'localhost' : options.host;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Lemme rebase and I'll push that.

@michael-ciniawsky
Copy link
Member

@wtgtybhertgeghgtwtg On last rebase please (promised) :)

addaleax pushed a commit to addaleax/webpack-dev-server that referenced this pull request Aug 28, 2018
@wtgtybhertgeghgtwtg @michael-ciniawsky
refactor(package): update internal-ip v1.2.0...3.0.0 (`dependencies…
418493d 
…`) (webpack#1471)
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 28, 2018 •
edited

Already landed as 418493d but I got 500 for this PR after clicking the merge button 🤷‍♂️ yesterday

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg deleted the bump-internal-ip branch August 28, 2018 17:03
@michael-ciniawsky michael-ciniawsky removed this from the 3.2.0 milestone Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes

Assignees
No one assigned
Labels
semver: patch status: approved type: refactor
Projects
No open projects
Dashboard
Refactor
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wtgtybhertgeghgtwtg @michael-ciniawsky @alexander-akait