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

do not swallow errors from http server. #1698

Closed
wants to merge 2 commits into from
Closed

do not swallow errors from http server. #1698

wants to merge 2 commits into from

Conversation

0xorial
Copy link

@0xorial 0xorial commented Mar 4, 2019

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

I was not sure where I can put a test for it. If someone can guide me I will be happy to add one.

Motivation / Use-Case

I was using WDS using node.js, made a trivial mistake (typed locahlost instead of localhost) and spent 10 minutes trying to find it.

Breaking Changes

None

Additional Info

@alexander-akait
Copy link
Member

Can you provide example where we can reproduce this problem, thanks for PR

@0xorial
Copy link
Author

0xorial commented Mar 4, 2019

      const config = require(configPath);
      const compiler = webpack(config);

      const log = require("webpack-log");

      const logger = log({
        name: "wds",
        level: "info",
        timestamp: true
      });
      const devServerInstance = new devServer(compiler, {}, logger);
      outputChannel.appendLine("about to listen...");

      // mistyped localhost
      devServerInstance.listen(8080, "locallllhost", (err: any) => {
        // never reached here :(
        if (err) {
          outputChannel.appendLine("listen error!");
          throw err;
        }
        outputChannel.appendLine('listening!');
      });

@0xorial
Copy link
Author

0xorial commented Mar 4, 2019

Just for some more context - I am trying to develop an extension for Visual Studio Code to integrate WDS.

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

Merging #1698 into master will decrease coverage by 0.12%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1698      +/-   ##
==========================================
- Coverage   83.76%   83.64%   -0.13%     
==========================================
  Files           8        8              
  Lines         536      538       +2     
  Branches      161      161              
==========================================
+ Hits          449      450       +1     
- Misses         70       71       +1     
  Partials       17       17              
Impacted Files Coverage Δ
lib/Server.js 78.74% <50.00%> (-0.16%) ⬇️

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 fa17131...6f64e54. Read the comment docs.

@alexander-akait
Copy link
Member

@0xorial can you rebase and add tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants