Skip to content

Commit

Permalink
fix: close compiler, own sh script and output clearing
Browse files Browse the repository at this point in the history
  • Loading branch information
@evenstensberg
evenstensberg committed Feb 5, 2019
1 parent 2ed8c60 commit 6ded275
Show file tree
Hide file tree
Showing 5 changed files with 645 additions and 674 deletions.
22 changes: 13 additions & 9 deletions bin/cli.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
"migrate",
"add",
"remove",
/*
"update",
"make",
*/
"serve",
"generate-loader",
"generate-plugin",
Expand Down Expand Up @@ -436,17 +432,17 @@ For more information, see https://webpack.js.org/api/cli/.`);
if (argv.w) {
compiler.hooks.watchRun.tap("WebpackInfo", compilation => {
const compilationName = compilation.name ? compilation.name : "";
console.log("\nCompilation " + compilationName + " starting…\n");
console.error("\nCompilation " + compilationName + " starting…\n");
});
} else {
compiler.hooks.beforeRun.tap("WebpackInfo", compilation => {
const compilationName = compilation.name ? compilation.name : "";
console.log("\nCompilation " + compilationName + " starting…\n");
console.error("\nCompilation " + compilationName + " starting…\n");
});
}
compiler.hooks.done.tap("WebpackInfo", compilation => {
const compilationName = compilation.name ? compilation.name : "";
console.log("\nCompilation " + compilationName + " finished\n");
console.error("\nCompilation " + compilationName + " finished\n");
});
}

Expand Down Expand Up @@ -491,8 +487,16 @@ For more information, see https://webpack.js.org/api/cli/.`);
process.stdin.resume();
}
compiler.watch(watchOptions, compilerCallback);
if (outputOptions.infoVerbosity !== "none") console.log("\nwebpack is watching the files…\n");
} else compiler.run(compilerCallback);
if (compiler.close) {
compiler.close(compilerCallback);
}
if (outputOptions.infoVerbosity !== "none") console.error("\nwebpack is watching the files…\n");
} else {
compiler.run(compilerCallback);
if (compiler.close) {
compiler.close(compilerCallback);
}
}
}

processOptions(options);
Expand Down
2 changes: 1 addition & 1 deletion bin/convert-argv.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ module.exports = function(...args) {
const ext = actualConfigFileName.replace(new RegExp(defaultConfigFileNames), "");
configFiles.push({
path: resolvedPath,
ext,
ext
});
}
}
Expand Down
Loading

6 comments on commit 6ded275

@Izhaki
Copy link

@Izhaki Izhaki commented on 6ded275 Mar 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@evenstensberg

I wonder why the change from console.log to console.error in this commit?

This change breaks the tests for us and while we can modify the tests, we now have no way to distinguish a real error from a log entry.

Conceptually I can't work out the sense in using console.error for what is clearly not an error.

Concretely, we have this code:

    this.output = [];
    this.errors = [];
    this.childProcess.stdout.on('data', (data) => {
      console.log(data.toString());
      this.output.push(data.toString());
    });
    this.childProcess.stderr.on('data', (data) => {
      console.log(data.toString());
      this.errors.push(data.toString());
    });

Followed by:

After(function () {
  kill(this.childProcess.pid, 'SIGINT');
  if (this.errors.length > 0) {
    throw new Error(`Errors: ${this.errors}`);
  }
});

@evenstensberg
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi. We did it because when you used --json to pipe your arguments to, it would include the console log. @jhnns has some resources on why console.error is the best case here

@Izhaki
Copy link

@Izhaki Izhaki commented on 6ded275 Mar 20, 2019

Choose a reason for hiding this comment

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

OK. So in the case of:

webpack --json > stats.json

Why not disable the console.log()?

I mean, isn't --json a way to request JSON output rather than console.log()?

I also wonder how you got around this in tests? Are you not testing cases where there was an error? Say a bad webpack config?

@evenstensberg
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that would be an option, but in watch, watch will either way output that we’re watching the files, which I want to have there. We changed the tests

@Izhaki
Copy link

@Izhaki Izhaki commented on 6ded275 Mar 20, 2019

Choose a reason for hiding this comment

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

So is this a done deal?

Asking so to know whether to go ahead and update our tests.

@evenstensberg
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Please sign in to comment.