fix: re-throw error to kill server

[emilyrohrbough]

• 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?

Yes

Motivation / Use-Case

Closes #2624

Re-throw an error to kill the server instead of logging and hanging.

Breaking Changes

Process will exit when an error occurs instead of hanging.

Additional Info

Terminal output when error is thrown:

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #2630 into v4 will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##               v4    #2630   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files          39       39           
  Lines        1308     1308           
  Branches      350      350           
=======================================
  Hits         1215     1215           
  Misses         89       89           
  Partials        4        4           

Impacted Files	Coverage Δ
lib/Server.js	96.33% <100.00%> (ø)

---

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 2090a41...9bc2ff8. Read the comment docs.

[emilyrohrbough]

Are these tests typically flaky? Is it possible to kick off another test run or did I miss a step required for tests?

[alexander-akait]

@emilyrohrbough should no, server is freeze without throw?

[emilyrohrbough]

Yes, without throwing the error, the server fails to start while the webpack compilation continues

[emilyrohrbough]

@evilebottnawi I reverted my changes to see if they had an impact on the test. It seems they are still failing. Do you have any insights on how to resolve these?

Client console.log › clientLogLevel is silent

TypeError: DirectoryWatcher is not a constructor

  at WatcherManager.Object.<anonymous>.WatcherManager.getDirectoryWatcher (node_modules/watchpack/lib/watcherManager.js:20:35)
  at WatcherManager.watchFile (node_modules/watchpack/lib/watcherManager.js:31:15)
  at Watchpack.<anonymous> (node_modules/watchpack/lib/watchpack.js:38:51)
      at Array.map (<anonymous>)
  at Watchpack.watch (node_modules/watchpack/lib/watchpack.js:37:29)
  at NodeWatchFileSystem.watch (node_modules/webpack/lib/node/NodeWatchFileSystem.js:72:18)

They don't seem to be related to my changes.

[Mayank1791989]

@evilebottnawi & @emilyrohrbough Any updates on this?

[alexander-akait]

@emilyrohrbough can you change branch on v4?

[Mayank1791989]

@evilebottnawi Do you think there is any point of adding error listener if we anyways throwing and killing the server, This will already happen if we don't add listener.

[alexander-akait]

@Mayank1791989 Some errors can be critical, some of them not, I think so, so we added it, but we probably need a little more tests, maybe you are right and we do not need a listener

[emilyrohrbough]

I could see value in both, depending on where / how this is being used, one could catch this error and handle it gracefully or just exit the process.

[alexander-akait]

@emilyrohrbough Yes, but in our case, we should log it, maybe it is unnecessary here

[emilyrohrbough]

Shall I remove the log?

[alexander-akait]

I think yes, because you already see error message 😄

[emilyrohrbough]

okay great! I'll make that change & fix these merge conflict soon.

[alexander-akait]

Thanks!

[alexander-akait]

/cc @hiroppy @Loonride

[hiroppy]

@emilyrohrbough Why did you update package-lock.json? I think it's unnecessary

[emilyrohrbough]

@hiroppy I did update it. I can revert to the locked in versions.

[emilyrohrbough]

@hiroppy It seems one or more of the locked in dependencies aren't compatible with node 12. I think the package-lock updates are needed.

[hiroppy]

@emilyrohrbough sorry I can't understand. This pr changed only this.

    this.listeningApp.on('error', (err) => {
      this.logger.error(err);
      throw err;
    });

And v4 branch's CI is fine v10, 12, and 14 even if we don't update package-lock.json.
see #2592

[emilyrohrbough]

@hiroppy I'm looking at #2592 and it looks like lint /the node 12.x is failing. Is that from something else?

[hiroppy]

@emilyrohrbough This is lint CI.

[emilyrohrbough]

Is it okay if lint on node 12 fails? That was the only check failing when the package-lock was not updated:

[hiroppy]

@emilyrohrbough

V4 branch has incorrect commit messages so CI isn't green.

Sorry, I misread, I thought your screenshot is v4...

[hiroppy]

Thank you for rerunning. ok, understood. So did you run npm ci && npm audit fix?

[emilyrohrbough]

@hiroppy Yes can do No I ran rm -rf node_modules; rm package-lock.json; npm install to regenerate. I will run npm ci && npm audit fix instead.

[hiroppy]

@emilyrohrbough Thanks! This PR looks good to me and CI is green.

[emilyrohrbough]

@hiroppy The build has passed. Are you okay with these changes?

[hiroppy]

thanks

[emilyrohrbough]

thank you!