Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Simplify error logging. #148

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Conversation

jaridmargolin
Copy link
Contributor

PENDING webpack/webpack#4568. Will also need to bump webpack version across packages.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Nice!

console.error(err.stack || err);
err.details && console.error(err.details);
});
errors.forEach(err => console.error(err));
Copy link
Member

Choose a reason for hiding this comment

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

errors.forEach(console.error);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is, forEach passes currentValue, index, array... We only want to log currentValue.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, always forget that. You can solve that by brining in unary from ramda and putting console.error in it:

const { unary } = require('ramda');

errors.forEach(unary(console.error));

Copy link
Member

Choose a reason for hiding this comment

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

Or if you prefer, put a logError or something in utils that has the same thing.

const logError = unary(console.error);

module.exports = {
  // ...
  logError,
  // ...
};

Copy link

@tj tj Mar 27, 2017

Choose a reason for hiding this comment

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

the rambda stuff seems not worth it in many cases IMO, adds an extra layer of indirection, making it harder to reason about if you're not familiar with them all. Some might be more obvious but if I saw unary() I'd just be like huh wtf is that if you're not familiar with the package

Copy link
Member

Choose a reason for hiding this comment

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

I agree that just slapping unary() right in the forEach probably isn't very descriptive, but putting it in utils with a comment should be OK. In fact, adding comments to all our utility functions would be a good thing. ™

err.details && console.error(err.details);
});

errors.forEach(err => console.error(err));
Copy link
Member

Choose a reason for hiding this comment

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

errors.forEach(console.error);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is, forEach passes currentValue, index, array... We only want to log currentValue.

@eliperelman
Copy link
Member

What will this do for exceptions that aren't bubbled from webpack, e.g. can't find a package?

@jaridmargolin
Copy link
Contributor Author

By default, console.error(new Error('test')) logs the stacktrace. The problem was that Webpack was adding custom properties to their Error types, and as a result the error was being logged as an object. The PR I submitted to webpack, was to correct the errors so that they log as they were intended.

In short, errors not bubbled from webpack should work just fine.

@eliperelman
Copy link
Member

@jaridmargolin any idea if this had shipped in Webpack yet?

@jaridmargolin
Copy link
Contributor Author

Merged but not shipped.

@eliperelman
Copy link
Member

Looks like Webpack 2.4.1 is out and I think it has your changes.

@jaridmargolin
Copy link
Contributor Author

Rebased and bumped webpack version to 2.4.1.

I did not make any changes to the forEach block discussed above. I think the current implementation errors.forEach(err => console.error(err)); is extremely readable, and introducing a util would be unnecessary indirection. If you are extremely opinionated here, I CAN make the change.. lmk

@codecov
Copy link

codecov bot commented Apr 16, 2017

Codecov Report

Merging #148 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files          36       36           
  Lines         512      512           
=======================================
  Hits          482      482           
  Misses         30       30

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 b3d3c73...95e8670. Read the comment docs.

@eliperelman
Copy link
Member

Looks like all the webpack versions were changed by modifying the version number in package.json, but this won't update what we keep locked in yarn. If you want to upgrade a package from the root, use:

yarn upgrade webpack
yarn deps:upgrade neutrino webpack
yarn deps:upgrade neutrino-middleware-chunk
# etc...

This will update package.json and yarn.lock files too.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Please run the relevant commands to update the yarn.lock files:

yarn upgrade webpack
yarn deps:upgrade neutrino-middleware-banner webpack
...

@jaridmargolin
Copy link
Contributor Author

yarnified 👍

@eliperelman
Copy link
Member

Looks like there's a merge conflict, could you rebase?

@eliperelman eliperelman merged commit e1e898f into neutrinojs:master Apr 25, 2017
@eliperelman
Copy link
Member

Released in v5.7.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants