Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

remove try/catch block to give better error messages#13

Merged
jhnns merged 1 commit intomasterfrom
better-errors
Jun 23, 2016
Merged

remove try/catch block to give better error messages#13
jhnns merged 1 commit intomasterfrom
better-errors

Conversation

@acidicX
Copy link
Copy Markdown
Contributor

@acidicX acidicX commented Jun 7, 2016

The try/catch blocks kills the stack trace required to find any errors in your webpack config, e.g. missing modules.

@benurb
Copy link
Copy Markdown
Contributor

benurb commented Jun 21, 2016

IMHO It's probably also ok to only alter err.message to something comprehensible and rethrow the error. This way the developer gets a better clue of what the problem is than with "Module xyz not found", but the stack trace still remains intact.

@acidicX
Copy link
Copy Markdown
Contributor Author

acidicX commented Jun 21, 2016

I think it's already comprehensible, as the stack trace will show that you messed up your webpack config :-) You should not throw an error you can't catch anyway...

A zillion different things could go wrong in your webpack config, there is no way to have a good error message in dyncfg at this point.

@meaku
Copy link
Copy Markdown
Member

meaku commented Jun 21, 2016

We added the try/catch for a reason... It was kind of hard to figure out whats wrong. Anyway we only modify it in case of a "file not found" error.

I agree with @acidicX that it's not ideal, but you should still be able to figure out webpack errors with the try/catch in place. Or is it a "MODULE_NOT_FOUND" error in any case?

@acidicX
Copy link
Copy Markdown
Contributor Author

acidicX commented Jun 22, 2016

Or is it a "MODULE_NOT_FOUND" error in any case?

Yep. That is the problem.

@jhnns
Copy link
Copy Markdown
Member

jhnns commented Jun 22, 2016

We added the try/catch for a reason... It was kind of hard to figure out whats wrong.

That was exactly the case with the current implementation because it was claiming that the config file was not found although it was in place. The error happened somewhere upstream but was obscured because any context information was missing.

We could alter the error message, but I'm not sure if that's necessary. IMHO the stack trace contains all the necessary information (even that the error happened in dynamic-config).

@jhnns jhnns merged commit 3dc997b into master Jun 23, 2016
@jhnns jhnns deleted the better-errors branch June 23, 2016 11:40
jhnns added a commit that referenced this pull request Jun 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants