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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix silent errors #476

Merged
merged 10 commits into from Feb 28, 2017

Conversation

Projects
None yet
4 participants
@calebmer
Copy link
Contributor

calebmer commented Feb 23, 2017

Logs an error to the console now instead of silently ignoring errors when a user does not explicitly use the error property on our data object.

This is one of the error changes we wanted to make for AC 1.0.

Interestingly enough, this change caught a couple of bugs in our test suite 馃槉

cc @helfer, @martijnwalraven

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@helfer helfer added the in progress label Feb 23, 2017

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 24, 2017

@calebmer this is great!

console.error('Uncaught (in react-apollo)', error.stack || error);
}
}, 10);
Object.defineProperty(data, 'error', {

This comment has been minimized.

@stubailo

stubailo Feb 24, 2017

Member

Would there be any benefit in only doing this in development mode? Or is the overhead minimal enough that we should just always do it?

This comment has been minimized.

@calebmer

calebmer Feb 27, 2017

Author Contributor

I鈥檓 for always logging because:

  1. The overhead is fairly small.
  2. In production a user should really be catching the error anyway.
  3. Some error tracking libraries may report console.error exceptions.
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 24, 2017

Are console logs still slow in react native?

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 27, 2017

@jbaxleyiii as far as I know it鈥檚 a safe assumption that React Native logs will always be slow 馃槪

However, for just printing an error stack trace which is a few bytes of text I highly doubt there will be any performance impact. The real killers for logging in React Native are sophisticated logging libraries like redux-logger which dump the entire Redux before/after state on every action.

Also, users should really be expected to catch and handle these errors, so even if there is some performance impact on the rare event there is an error it is a (mostly) one time cost and it has an easy fix on the user鈥檚 part.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 27, 2017

That makes total sense!

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 27, 2017

Fixed CI 馃憤

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Feb 28, 2017

Neat! We've been talking about doing this for a while, but we never actually did it 馃憤

@helfer helfer merged commit a30cb83 into master Feb 28, 2017

5 checks passed

./dist/index.min.js +189 bytes (+0.67%) 鈫 28,232 bytes
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.8%) to 93.128%
Details

@helfer helfer removed the in progress label Feb 28, 2017

@helfer helfer deleted the fix/silent-errors branch Feb 28, 2017

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