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

Proposal to enhance loader error reporting API: structured error reporting #2878

Closed
andreypopp opened this issue Aug 16, 2016 · 8 comments · Fixed by #6542

Comments

@andreypopp
Copy link
Contributor

commented Aug 16, 2016

Problem

Currently loader context provides an opaque API for error reporting: only
message , stack and (undocumented?) hideStack error object
properties are used to report an error to a user.

Thus the appearance of error varies widely between different loaders. Babel,
eslint, css, postcss, sass loaders for example all have different error
reporting styles.

In addition to that there's a lot of duplicated info, some loaders render
filename of the module which is unnecessary as Webpack does so too and in a more
concise manner (it uses request shortener).

I believe this overwhelms users of Webpack and takes a lot more cognitive efforts
to recognize and process errors.

Solution

The solution I propose is to enhance error reporting API of loaders, namely:

this.emitError(err)                     // emit error
let cb = this.async(); cb(err, result)  // emit error via cb
throw new Error(...)                    // emit error via throw

To accept an Error object with the following properties:

  • name: string — name of the error. Already present on errors but is not used explicitly.
  • reason: string — a human readable error message without filename of the
    module happened into. If not provided then message property is used as
    before.
  • loc?: {line: number, column: number}optional default: undefined, a
    location within the module error happened into. If provided it is used to
    render a code frame (similar to how babel-loader and css-loader do).
  • hideStack?: booleanoptional, default false, a boolean flag which
    indicates that webpack shouldn't render stack trace. Note that hideStack
    already works but is undocumented.

With presence of such structured error messages Webpack could render errors in a
consistent way:

ERROR in {moduleName}
${err.name}: ${err.reason}

  ${err.loc ? formatCodeFrame(source, err.loc.line, err.loc.column) : ''}

Note that moduleName and source are available to Webpack already.

Also webpack could provide plugin hooks for different error reporting UIs.

Example of loader API usage:

try {
  return babelTransform(source)
catch (internalError) {
  // only normalize known errors
  if (internalError instanceof BabelSyntaxError) {

    let error = new Error()

    // readable name of the error type
    error.name = 'Syntax error';

    // strip filename from here if needed or use some other property
    error.reason = internalError.message;

    // location
    error.loc = internalError.location;

    // stack trace from parser isn't useful for end users
    error.hideStack = true;

    throw error;

  // throw all other errors as-is
  } else {
    throw internalError;
  }
}

Alternative solutions

Such work could be done by loaders themselves (normalizing message property
and formatting code frames). But:

  1. This is more fragile as it is easy to drift away from
    consistence if error appearance would be controlled separately by loaders.
  2. Loader will have to do more to get better error reporting.
  3. Webpack provides UI but leaves error reporting UI to loaders. This leads to
    inconsistent developer experience.

I made the following PRs to loaders:

But I believe if we enhance error reporting API it would be a much cleaner
solution.

Also see what create-react-app is doing: we can get rid of this with this proposal & a couple of PRs to loadrs too.

@bebraw bebraw added the enhancement label Aug 16, 2016

@TheLarkInn

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

Did we discuss we would want loader name reported as well?

@andreypopp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

@TheLarkInn yep, totally. Webpack is already aware of that info: it could print loader request.

@TheLarkInn

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

💯. A nice to have but can this be architected as a default template? That can be plugged into? Or enforce the default error layout. I'm okay with either decision.

@andreypopp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

@TheLarkInn as I understood current webpack UI isn't written with plugins in mind (only compiler allows plugins). I think the first step would be to improve on default UI and only then modify it to allow plugins.

@dmitriid

This comment has been minimized.

Copy link

commented Nov 2, 2016

Also:

  • an error in loader:
    -- should not pollute output with stacktraces from loader
    -- should not pollute output with stacktraces from webpack
  • an error reported by loader:
    -- should be clearly shown as produced by the loader
  • an error reported by webpack:
    -- should be clearly shown as produced by webpack
@webpack-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

This issue had no activity for at least half a year.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@zyrolasting

This comment has been minimized.

Copy link

commented Aug 25, 2017

Commenting to remove inactive label due to interest and pending contribution.

@mc-zone

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

I've make a PR #2878 to add loader name to error message internally so that people can see what the error comes from clearly.

About the properties mentioned above, on the one hand there maybe duplicated (name, reason with message and details), on the other hand I just realized (by sokra 's hint in my PR) that if an Error has got these props, it may have unexpected effect when we using it directly. So hope we can discuss more about that.

@sokra sokra closed this in #6542 Jun 6, 2018

sokra added a commit that referenced this issue Jun 6, 2018
Merge pull request #6542 from mc-zone/feature/module-build-error-with…
…-loader-name

Add loader name to error message. Resolves #2878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.