Skip to content

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

Closed
wtgtybhertgeghgtwtg wants to merge 1 commit into
webpack:masterfrom
wtgtybhertgeghgtwtg:bump-internal-ip
Closed

refactor(package): update internal-ip v1.2.0...3.0.0 (dependencies)#1471
wtgtybhertgeghgtwtg wants to merge 1 commit into
webpack:masterfrom
wtgtybhertgeghgtwtg:bump-internal-ip

Conversation

@wtgtybhertgeghgtwtg

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

Copy link
Copy Markdown
Contributor

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

michael-ciniawsky commented Aug 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 approved these changes Aug 26, 2018
View reviewed changes

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

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Aug 26, 2018 •
edited
Loading

Copy link
Copy Markdown

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

michael-ciniawsky commented Aug 26, 2018

Copy link
Copy Markdown
Contributor 
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

wtgtybhertgeghgtwtg commented Aug 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

wtgtybhertgeghgtwtg commented Aug 26, 2018

Copy link
Copy Markdown
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
Comment thread lib/util/createDomain.js Outdated
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;

@alexander-akait alexander-akait Aug 27, 2018

Copy link
Copy Markdown
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;?

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg Aug 27, 2018

Copy link
Copy Markdown
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

michael-ciniawsky commented Aug 27, 2018

Copy link
Copy Markdown
Contributor

@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

michael-ciniawsky commented Aug 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
+1 more reviewer
@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver: patch status: approved type: refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wtgtybhertgeghgtwtg @michael-ciniawsky @alexander-akait