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

Add the message in BabelLoaderError's stack trace #499

Merged
merged 2 commits into from
Aug 18, 2017
Merged

Add the message in BabelLoaderError's stack trace #499

merged 2 commits into from
Aug 18, 2017

Conversation

jennings
Copy link
Contributor

@jennings jennings commented Aug 15, 2017

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When a BabelLoaderError is thrown on Node 8.x, the message is not included in the stack property. When webpack wraps the error in a ModuleBuildError, the message is lost from webpack's output:

ERROR in ./entry.js
Module build failed: Error
    at transpile (/Users/stephen/t/node_modules/babel-loader/lib/index.js:64:13)
    at Object.module.exports (/Users/stephen/t/node_modules/babel-loader/lib/index.js:174:20)

What is the new behavior?

The BabelLoaderError's message is included in the stack property, so even after wrapping in a ModuleBuildError, the error message and the code where the error occurred are visible.

ERROR in ./entry.js
Module build failed: /Users/stephen/t/entry.js: Reference to undeclared variable "foo"

> 1 | foo()
    | ^
  2 |
Error
    at transpile (/Users/stephen/t/node_modules/babel-loader/lib/index.js:64:13)
    at Object.module.exports (/Users/stephen/t/node_modules/babel-loader/lib/index.js:174:20)

Does this PR introduce a breaking change?

  • Yes
  • No

Other information:

I had first thought this was a webpack issue, but they suggested this was something the loader should change.

I think the behavior of Error.captureStackTrace may have changed between Node versions, because this behavior appeared when I upgraded from Node 6.x to 8.x. Perhaps the old behavior was to not read the message property until stack was read, and the new behavior is to read message immediately? I have not had time to verify this, though, this is just speculation.

This is the webpack.config.js I was using to create the error messages above (entry.js simply contains the text foo()):

var path = require('path')

module.exports = {
  entry: path.join(__dirname, 'entry.js'),

  output: {
    filename: 'output.js'
  },

  module: {
    rules: [
      {
        test: /\.js$/,
        use: {
          loader: 'babel-loader',
          options: {
            plugins: ['babel-plugin-undeclared-variables-check']
          }
        }
      }
    ]
  }
}

When Error.captureStackTrace is called at the top of BabelLoaderError,
the error name and message are not added to the stack property. When
webpack wraps this in a ModuleBuildError, the message is lost.

Capturing at the end of the function ensures the message is part of the
stack trace, so it is eventually added to webpack's output.
@danez
Copy link
Member

danez commented Aug 18, 2017

Thanks. I think this is because of this issue: https://bugs.chromium.org/p/v8/issues/detail?id=5962 So v8 started formating the stack sync when calling captureStackTrace which accesses this.message, but we are setting it after calling captureStackTrace.

This will be fixed in some upcoming node version, but to be safe we can merge this.

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

Successfully merging this pull request may close these issues.

2 participants