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

use webpack for logging #55

Merged
merged 4 commits into from
Jun 14, 2021
Merged

use webpack for logging #55

merged 4 commits into from
Jun 14, 2021

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented May 18, 2021

use the webpack infrastructure logger (if available) rather than outputting to the console directly so that webpack --silent mode is respected.

Goal

  • Webpack supports a --silent option and our webpack plugins should respect that
  • In webpack 4 it is common to output stats as json and pipe it to a file (e.g. webpack --mode=production --profile --json > stats/stats.json). This requires silent mode to be respected. Note: webpack 5 stats don't need piping as they are generated withnpx webpack --profile --json=compilation-stats.json. See https://webpack.js.org/api/stats/

Design

Use the webpack recommended way of handling logging in webpack 4 and 5 plugins. See https://v4.webpack.js.org/api/plugins/#logging and https://webpack.js.org/api/plugins/#logging

The logger returned with getLogger doesn't output anything to the console in the event of an error, so getInfrastructureLogger is used, as the output is more useful.

BugsnagBuildReporterPlugin will not use the webpack logger when a custom logger option is supplied.

webpack <= 4.37 doesn't have the logging API so:

  • BugsnagBuildReporterPlugin falls back to its built in logger (unless a custom option logger is supplied); and
  • BugsnagSourceMapUploaderPlugin falls back to console (there is no custom logger option).

Changeset

  • BugsnagBuildReporterPlugin: use the webpack "infrastructure" logger for logging, if present
  • BugsnagSourceMapUploaderPlugin: use the webpack "infrastructure logger" for logging, if present

Testing

Output from webpack 4:

Before

[bugsnag-build-reporter] Configuration error
[bugsnag-build-reporter]   sourceControl must be an object containing { provider, repository, revision } (got "{
  provider: 'github',
  repository: 'https://github.com/bugsnag/dashboard-js',
  revision: undefined
}")
[BugsnagSourceMapUploaderPlugin] uploading sourcemap for "http://localhost:8080/assets/app.826c2f1ffce9f1548c4c.js"
Error: HTTP status 401 received from upload API
    at /Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/dist/Request.js:105:33
    at ConcatStream.<anonymous> (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/concat-stream/index.js:37:43)
    at ConcatStream.emit (node:events:381:22)
    at finishMaybe (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/readable-stream/lib/_stream_writable.js:620:14)
    at afterWrite (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/readable-stream/lib/_stream_writable.js:466:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
error Command failed with exit code 1.

After:

<e> [BugsnagBuildReporterPlugin] Configuration error
<e> [BugsnagBuildReporterPlugin]   sourceControl must be an object containing { provider, repository, revision } (got "{
<e>   provider: 'github',
<e>   repository: 'https://github.com/bugsnag/dashboard-js',
<e>   revision: undefined
<e> }")
Error: HTTP status 401 received from upload API
    at /Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/dist/Request.js:105:33
    at ConcatStream.<anonymous> (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/concat-stream/index.js:37:43)
    at ConcatStream.emit (node:events:381:22)
    at finishMaybe (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/readable-stream/lib/_stream_writable.js:620:14)
    at afterWrite (/Users/dan/dashboard-js/node_modules/@bugsnag/source-maps/node_modules/readable-stream/lib/_stream_writable.js:466:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
error Command failed with exit code 1.

Note the absence of [BugsnagSourceMapUploaderPlugin] uploading sourcemap for "http://localhost:8080/assets/app.826c2f1ffce9f1548c4c.js" as the webpack logger does not output log level.

If it was a warn it would output <w> [BugsnagSourceMapUploaderPlugin] uploading sourcemap for "http://localhost:8080/assets/app.826c2f1ffce9f1548c4c.js" (in yellow colour).

It might be worth reviewing the various levels used throughout the code.

In terms of unit-level coverage I think the first step would be to convert to jest


const chunkToSourceMapDescriptors = chunk => {
// find .map files in this chunk
const maps = chunk[webpackMajorVersion >= 5 ? 'auxiliaryFiles' : 'files'].filter(file => /.+\.map(\?.*)?$/.test(file))

if (!publicPath) {
console.warn(`${LOG_PREFIX} ${PUBLIC_PATH_WARN}`)
logger.warn(`${LOG_PREFIX} ${PUBLIC_PATH_WARN}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs a little more work as webpack already adds the prefix so we need to omit it when the webpack logger is used

build-reporter-plugin.js Show resolved Hide resolved
@djskinner djskinner force-pushed the use-webpack-for-logging branch 2 times, most recently from 9dee51f to d793f93 Compare May 19, 2021 08:35
: {}
)

reportBuild(this.build, options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that for webpack >= 4.37 the logLevel documented option does nothing because the built-in logger is not used. This also applies (as it did before) if you pass a custom logger. Not sure if this is worth documenting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine. We should update the docs to say which cases it works in.

rather than outputting to the console directly so that the plugins respect webpack silent mode
@djskinner djskinner force-pushed the use-webpack-for-logging branch from d793f93 to 71fbbab Compare May 19, 2021 09:11
@djskinner djskinner marked this pull request as ready for review May 20, 2021 16:57
@djskinner djskinner requested a review from bengourley May 20, 2021 16:57
Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

It would seem odd to have to use a "warn" log to say that an upload was successful. Can you use .info() instead?

: {}
)

reportBuild(this.build, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine. We should update the docs to say which cases it works in.

@djskinner djskinner force-pushed the use-webpack-for-logging branch from 5b317d0 to 83062c3 Compare June 8, 2021 15:56
@@ -91,7 +93,7 @@ class BugsnagSourceMapUploaderPlugin {

const sourceMaps = stats.chunks.map(chunkToSourceMapDescriptors).reduce((accum, ds) => accum.concat(ds), [])
parallel(sourceMaps.map(sm => cb => {
console.log(`${LOG_PREFIX} uploading sourcemap for "${sm.url}"`)
logger.info(`${logPrefix}uploading sourcemap for "${sm.url}"`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to info so that it shows in the webpack logger output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Output shows as:

<i> [BugsnagSourceMapUploaderPlugin] uploading sourcemap for "http://localhost:8080/assets/slack.826c2f1ffce9f1548c4c.js"

@djskinner djskinner merged commit 4167102 into master Jun 14, 2021
@djskinner djskinner deleted the use-webpack-for-logging branch June 14, 2021 10:27
This was referenced Jul 6, 2021
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