Add hook to handleError. #485

squaresurf · 2016-07-21T18:17:01Z

Why:

It is hard to send exceptions to an exception tracker (e.g. Rollbar)
when they're all caught and sent to console.error.

This change addresses the need by:

Update handleError to check for the existence of a logException
function on the window, then pass the exception to it if it exists.

This change is

justin808 · 2016-07-22T03:42:01Z

Great idea!

See comment below about using https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

We also need tests for this, and update /spec/dummy to use this.

Thanks!

Justin

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

node_package/src/handleError.js, line 67 [r1] (raw file):

  }

  if (typeof window.logException === 'function') {

Rather than a global, how about allowing ReactOnRails.setOptions to take a custom function, like this:

(documentation: https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md)

ReactOnRails.setOptions( {
  traceTurbolinks: false,

  exceptionLogger(e) {
     console.error('ReactOnRails: e = ', e);
     MyCustomNotificationSystem.notifyError(e); // Something custom to your app.
  }
});

Comments from Reviewable

justin808 · 2016-08-05T06:13:28Z

@squaresurf Any update?

squaresurf · 2016-08-05T13:52:24Z

Hey @justin808, I'm sorry for the lag. I'm going to work on this today 😄.

squaresurf · 2016-08-05T19:15:14Z

@justin808 will a rebase off of master and a force push of this branch mess up your review process or other users at all? I can do a merge commit, but that will add an unnecessary commit into the history.

justin808 · 2016-08-06T08:17:59Z

rebasing does not hurt the review process

please do so!

justin808 · 2016-08-07T05:59:42Z

@squaresurf Let's get this one in. I want to push out a new release with this one!

squaresurf · 2016-08-08T04:06:21Z

Ok, @justin808 I've updated this PR. Let me know your thoughts.

justin808 · 2016-08-08T06:16:25Z

@squaresurf Please address the CI issue:

Using react_on_rails 6.0.5 from source at `../..` Bundle complete! 33 Gemfile dependencies, 114 gems now installed. Use `bundle show [gemname]` to see where a bundled gem is installed. node_package/src/ReactOnRails.js -> node_package/lib/ReactOnRails.js node_package/src/RenderUtils.js -> node_package/lib/RenderUtils.js node_package/src/StoreRegistry.js -> node_package/lib/StoreRegistry.js node_package/src/buildConsoleReplay.js -> node_package/lib/buildConsoleReplay.js node_package/src/ComponentRegistry.js -> node_package/lib/ComponentRegistry.js node_package/src/clientStartup.js -> node_package/lib/clientStartup.js node_package/src/context.js -> node_package/lib/context.js node_package/src/createReactElement.js -> node_package/lib/createReactElement.js node_package/src/ReactOnRails.js -> node_package/lib/ReactOnRails.js node_package/src/generatorFunction.js -> node_package/lib/generatorFunction.js node_package/src/RenderUtils.js -> node_package/lib/RenderUtils.js node_package/src/ComponentRegistry.js -> node_package/lib/ComponentRegistry.js node_package/src/StoreRegistry.js -> node_package/lib/StoreRegistry.js node_package/src/handleError.js -> node_package/lib/handleError.js node_package/src/isRouterResult.js -> node_package/lib/isRouterResult.js node_package/src/scriptSanitizedVal.js -> node_package/lib/scriptSanitizedVal.js node_package/src/buildConsoleReplay.js -> node_package/lib/buildConsoleReplay.js node_package/src/serverRenderReactComponent.js -> node_package/lib/serverRenderReactComponent.js npm WARN deprecated babel@6.5.2: Babel's CLI commands have been moved from the babel package to the babel-cli package node_package/src/clientStartup.js -> node_package/lib/clientStartup.js node_package/src/ReactOnRails.js -> node_package/lib/ReactOnRails.js node_package/src/context.js -> node_package/lib/context.js node_package/src/RenderUtils.js -> node_package/lib/RenderUtils.js node_package/src/createReactElement.js -> node_package/lib/createReactElement.js node_package/src/generatorFunction.js -> node_package/lib/generatorFunction.js node_package/src/StoreRegistry.js -> node_package/lib/StoreRegistry.js node_package/src/ComponentRegistry.js -> node_package/lib/ComponentRegistry.js npm ERR! tar pack Error reading /home/travis/build/shakacode/react_on_rails npm ERR! addLocalDirectory Could not pack /home/travis/build/shakacode/react_on_rails to /home/travis/.npm/react-on-rails/6.0.5/package.tgz npm ERR! addLocal Could not install /home/travis/build/shakacode/react_on_rails npm ERR! Linux 3.19.0-30-generic npm ERR! argv "/home/travis/.nvm/versions/node/v5.10.0/bin/node" "/home/travis/.nvm/versions/node/v5.10.0/bin/npm" "install" npm ERR! node v5.10.0 npm ERR! npm v3.10.5 npm ERR! path /home/travis/build/shakacode/package/node_package/lib/ComponentRegistry.js npm ERR! Didn't get expected byte count npm ERR! expect: 0 npm ERR! actual: 2287 npm ERR! npm ERR! If you need help, you may report this error at: npm ERR! https://github.com/npm/npm/issues node_package/src/buildConsoleReplay.js -> node_package/lib/buildConsoleReplay.js npm ERR! Please include the following file with any support request: npm ERR! /home/travis/build/shakacode/react_on_rails/gen-examples/examples/basic/client/npm-debug.log node_package/src/handleError.js -> node_package/lib/handleError.js rake aborted! Command failed with status (1): [cd /home/travis/build/shakacode/react_on_r...] /home/travis/build/shakacode/react_on_rails/rakelib/task_helpers.rb:20:in `block in sh_in_dir' /home/travis/build/shakacode/react_on_rails/rakelib/task_helpers.rb:20:in`each' /home/travis/build/shakacode/react_on_rails/rakelib/task_helpers.rb:20:in `sh_in_dir' /home/travis/build/shakacode/react_on_rails/rakelib/examples.rake:46:in`block (3 levels) in ' /home/travis/build/shakacode/react_on_rails/rakelib/examples.rake:34:in `block (4 levels) in ' Tasks: TOP => examples:build_webpack_bundles_basic => examples:npm_install_basic (See full trace by running task with --trace) node_package/src/isRouterResult.js -> node_package/lib/isRouterResult.js The command "rake examples" failed and exited with 1 during . Your build has been stopped. node_package/src/scriptSanitizedVal.js -> node_package/lib/scriptSanitizedVal.js

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

justin808 · 2016-08-08T06:48:08Z

We need more tests of this and for this to be confirmed as working via a test in spec/dummy.

Ideally, I'd like both the default and the customized one tested.

We also need to update README.md and CHANGELOG.md.

See https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

node_package/src/handleError.js, line 67 [r2] (raw file):

  }

  ReactOnRails.options('excptionLogger')(e);

spelling error

was this caught in the tests?

and what if this option is not set

node_package/src/ReactOnRails.js, line 15 [r2] (raw file):

const DEFAULT_OPTIONS = {
  traceTurbolinks: false,
  exceptionLogger: function(e) {}

The default should be the current functionality.

node_package/src/ReactOnRails.js, line 60 [r2] (raw file):

   * Available Options:
   * `traceTurbolinks: true|false Gives you debugging messages on Turbolinks events
   * `exceptionLogger: function Gets passed the exception as the first argument

in the docs, put in the default behavior if this option is not set.

We also need to update this file: https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

node_package/tests/ReactOnRails.test.js, line 49 [r2] (raw file):

  assert.plan(1);
  const logger = function() {};
  ReactOnRails.setOptions({ exceptionLogger: logger });

we should confirm that the logger is a function

spec/dummy/client/app/startup/clientRegistration.jsx, line 19 [r2] (raw file):

  traceTurbolinks: true,
  exceptionLogger: function(e) {
    console.log('Log from exceptionLogger:', e);

We need to preserve the existing functionality if this is not set.

Comments from Reviewable

justin808 · 2016-08-08T09:56:36Z

I'd like to get this in the 6.1 release this coming week.

squaresurf · 2016-08-08T16:44:03Z

@justin808 great catches. To be honest, I'm not sure I'll be able to finish it up this week as I have a lot on my plate currently. Please feel free to take over this PR if you'd like to push it forward.

squaresurf · 2016-08-08T16:51:02Z

To be very clear, I should have some time on Friday to work on this.

squaresurf · 2016-08-08T16:54:11Z

I just fixed the typo. We'll see what I can do on Friday. Please feel free to take over if you need this done before then.

justin808 · 2016-08-12T03:52:33Z

Hi @squaresurf Thanks for helping with this issue. We've got a number of nice enhancements planned for the 6.1 release this weekend.

squaresurf · 2016-08-12T03:54:29Z

Awesome! I'm hoping to spend some time on this tomorrow! 😸

Why: * It is hard to send exceptions to an exception tracker (e.g. Rollbar) when they're all caught and sent to console.error. This change addresses the need by: * Update handleError to check for the existence of a logException function on the window, then pass the exception to it if it exists.

squaresurf · 2016-08-12T15:50:31Z

Review status: 3 of 4 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

node_package/src/ReactOnRails.js, line 15 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

The default should be the current functionality.

This is the current functionality, that is it does not change how the handleError.js is handling errors. By setting the default to an empty function, it then allows us to call the `exceptionLogger` without having to check the value. It's a way to make sure we can "Tell, don't ask."

spec/dummy/client/app/startup/clientRegistration.jsx, line 19 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

We need to preserve the existing functionality if this is not set.

I can remove this if you'd like. I had just added it in as an example of what could be done by adding an `exceptionLogger`.

Comments from Reviewable

Why: * We should be sure that any exceptionLogger option passed to us is a function. This change addresses the need by: * Add a check to make sure the exceptionLogger option is a function.

coveralls · 2016-08-12T17:05:45Z

Coverage remained the same at 82.36% when pulling 48bcbbd on spartansystems:js-exception-hook into a6373f0 on shakacode:master.

squaresurf · 2016-08-12T17:08:43Z

@justin808 would you please take another look? I tried to respond to everything on reviewable, so let me know if I missed any items. I will squash the commits once we're ready to merge.

Also, feel free to modify this in any way you see fit. The main driver for this PR is that I need a way to pass exceptions to Rollbar. We're using my fork at the moment so no rush on our side.

justin808 · 2016-08-14T20:39:46Z

@squaresurf Let me know if you still want to finish this one up. This might wait until the 6.2 release if we can't get this done in the next couple of days.

@alexfedoseev @robwise @dylangrafmyre Given our usage of honeybadger, what are your thoughts on this one?

alex35mil · 2016-08-15T05:39:38Z

@justin808 I haven't looked into the details, but from the consumer perspective AFAIU I just pass exceptionLogger: errorTracker.notify and gem will trigger it on exception. If it works like this, then LGTM.

squaresurf · 2016-08-15T17:56:28Z

@justin808 I'll do my best to address your comments today.

@alexfedoseev the way I have it setup, errorTracker.notify would need to accept the exception as the first argument. Please see how we call the logger in handleError.js

robwise · 2016-08-16T05:33:49Z

@squaresurf Why don't we just not catch the exception if client rendering?

justin808 · 2016-08-16T05:41:49Z

@robwise This is how we handle errors:

https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js#L81

handleError does this: https://github.com/shakacode/react_on_rails/blob/master/node_package/src/handleError.js#L38

robwise · 2016-08-16T11:15:07Z

@justin808 right, err I'm saying just delete the try/catch block with handleErrors and the problem is solved?

squaresurf · 2016-08-16T15:50:32Z

@robwise I'd love it if the errors weren't caught. Honestly I tried to add a simple function in a small way so as to not rewrite how you all were doing things. I just need a way to send my exceptions onto an exception tracker (e.g. Rollbar). Preferably ReactOnRails wouldn't catch exceptions only to console.log them and would allow me to handle them, but like I said, I didn't want to fundamentally change how it worked. I had assumed it would be easiest to get a simple config through, but that is proving to be very incorrect.

At this point in time, I would like for one of you to take this over and accomplish the task to allow for a way to send exceptions onto an exception tracker in whatever fashion will pass all the checks and balances that governs this project. I have a fork that does what I need for now so feel free to take your time.

justin808 · 2016-08-16T17:41:55Z

@squaresurf @robwise how about this? We just throw if not on the server side.

@robwise If we did this, would we need a major version bump? Maybe not.

Change lines:

  } else {
     // We might have an option to not throw here. We definitely cannot throw server side, or
     // else we don't get a useful error message.
     throw e;
  }

Full code

export default (options) => {
  const { e, jsCode, serverSide } = options;

  console.error('Exception in rendering!');

  let msg = handleGeneratorFunctionIssue(options);

  if (jsCode) {
    console.error(`JS code was: ${jsCode}`);
  }

  if (e.fileName) {
    console.error(`location: ${e.fileName}:${e.lineNumber}`);
  }

  console.error(`message: ${e.message}`);
  console.error(`stack: ${e.stack}`);

  if (serverSide) {
    msg += `Exception in rendering!
${e.fileName ? `\nlocation: ${e.fileName}:${e.lineNumber}` : ''}
Message: ${e.message}
${e.stack}`;

    const reactElement = React.createElement('pre', null, msg);
    return ReactDOMServer.renderToString(reactElement);
  } else {
     // We might have an option to not throw here. We definitely cannot throw server side, or
     // else we don't get a useful error message.
     throw e;
  }
};

squaresurf · 2016-08-16T17:44:37Z

That looks good to me. Thank you @justin808

I'd love to know what it would take to get server side exceptions into a tracker as well. Something to think about for the future.

justin808 · 2016-08-16T19:49:26Z

@squaresurf We already have that option:

https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/config/initializers/react_on_rails.rb#L63

  config.raise_on_prerender_error = false # change to true to raise exception on server if the JS code throws

squaresurf · 2016-08-16T20:15:01Z

Oh that's right @justin808. I forgot about that. Thank you for reminding me.

justin808 · 2016-08-17T08:43:28Z

@dzirtusss or @AlexKVal Any chance that either of you can get this one to the finish line. I'd like to ship a 6.1 version soon!

dzirtusss · 2016-08-17T14:30:29Z

I can check this, but as I understand I can't commit to this PR and need to open new one?
Anyway I forked branch from this PR to https://github.com/dzirtusss/react_on_rails/tree/js-exception-hook

dzirtusss · 2016-08-17T16:50:51Z

Review status: 1 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

node_package/src/ReactOnRails.js, line 15 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

So we're appending behavior rather than replacing the behavior?

Done

node_package/src/ReactOnRails.js, line 15 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

This would look better as a fat arrow function.
 exceptionLogger: (e) => { }
but we're not going to do this...

We'll make the default null;

Done

node_package/tests/ReactOnRails.test.js, line 49 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

we should confirm that the logger is a function

Done

Comments from Reviewable

dzirtusss · 2016-08-17T17:14:24Z

Review status: 1 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

docs/api/javascript-api.md, line 35 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

see Api comments below

and we can add the trailing back ticks, right? this is a MD file.

Done

node_package/src/ReactOnRails.js, line 60 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

see comment above

Done

Comments from Reviewable

justin808 · 2016-08-17T18:44:06Z

@dzirtusss The idea is to make a new PR with the code I suggested, which throws on the client side. So there's no options and no change to the README. We should throw a new exception that contains the string created from the original exception. The exception thrown should also contain a property called cause which is the original exception. Please feel free to contact me.

Review status: 1 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Comments from Reviewable

squaresurf · 2016-08-17T18:45:39Z

@justin808 will it be possible to get the original stack trace from the cause property?

justin808 · 2016-08-17T19:12:23Z

See #485 (comment)

original stack trace is in the top level message and can be from the cause

robwise · 2016-08-17T23:50:26Z

@justin808 that code doesn't look right to me. All we need to do is delete the try/catch from here: https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js#L80-L86

Then you are done and this problem is solved.

justin808 · 2016-08-18T00:23:18Z

@robwise if you throw on the server side, you basically seg fault and you're hosed.

robwise · 2016-08-18T02:07:27Z

@justin808 the file I linked has nothing to do with server rendering, though. It is only for client rendering, which is where we have no reason to be catching errors and is what is causing the entire problem. We know it is only used for client rendering because:

the name is client startup
it is currently hard-coding server-side: false
it is using document which doesn't exist when server rendering

justin808 · 2016-08-18T04:42:45Z

@robwise, you're right about being only used for client rendering. However, the current error handler adds some information, such as the name of the object being rendered. We might someday have other information in the error that comes from ReactOnRails.

robwise · 2016-08-18T05:32:00Z

@justin808

We might someday have other information in the error that comes from ReactOnRails.
YAGNI

the current error handler adds some information, such as the name of the object being rendered

You get that from the stack trace anyway, because the component is the thing that's going to be causing the error.

So it's not adding any benefit, and it is breaking peoples' error handling functionality, let's delete it.

justin808 · 2016-08-18T05:58:23Z

@robwise We're missing logging the component name (might not match up to the component file), and we also detect if we are incorrectly determining a React class versus a stateless functional component.

robwise · 2016-08-18T08:50:54Z

@justin808 Do you mean this? That code is just checking the already thrown, descriptive error. It should be deleted as well. I think this is a no-brainer solution.

justin808 · 2016-08-18T08:58:56Z

That code is just checking the already thrown, descriptive error. It should be deleted as well

That code catches the case of when we fail to determine correctly if we're looking at a React class or function that returns a React class. Yes, we don't have this in the API anymore, but it sure doesn't hurt to check the exception, especially for server rendering. The error is super obscure.

@dzirtusss is doing as @robwise suggested, mostly, with a couple tweaks, in #521. @robwise take a look when you get a chance.

justin808 · 2016-08-20T06:58:56Z

See #521

squaresurf · 2016-08-22T14:09:17Z

Thank you all!

justin808 added the waiting for submitter label Jul 22, 2016

squaresurf force-pushed the js-exception-hook branch from 83575cb to 5ca8b16 Compare August 8, 2016 04:05

justin808 modified the milestone: 6.1 Aug 8, 2016

squaresurf force-pushed the js-exception-hook branch from 5ca8b16 to 6b8437b Compare August 8, 2016 16:52

justin808 added the enhancement label Aug 10, 2016

squaresurf added 2 commits August 12, 2016 06:33

Fix javascript linter warnings.

e45bf67

squaresurf added 2 commits August 12, 2016 09:11

Add documentation.

47f376a

Validate that exceptionLogger is a function.

df177b0

Why: * We should be sure that any exceptionLogger option passed to us is a function. This change addresses the need by: * Add a check to make sure the exceptionLogger option is a function.

squaresurf force-pushed the js-exception-hook branch from 6b8437b to df177b0 Compare August 12, 2016 16:34

Fix line length violations.

48bcbbd

dzirtusss mentioned this pull request Aug 17, 2016

Pass-through JS error to client #521

Merged

justin808 closed this Aug 20, 2016

Add hook to handleError. #485

Add hook to handleError. #485

Conversation

squaresurf commented Jul 21, 2016 • edited by justin808 Loading

justin808 commented Jul 22, 2016

justin808 commented Aug 5, 2016

squaresurf commented Aug 5, 2016

squaresurf commented Aug 5, 2016

justin808 commented Aug 6, 2016

justin808 commented Aug 7, 2016

squaresurf commented Aug 8, 2016

justin808 commented Aug 8, 2016

justin808 commented Aug 8, 2016

justin808 commented Aug 8, 2016

squaresurf commented Aug 8, 2016

squaresurf commented Aug 8, 2016

squaresurf commented Aug 8, 2016

justin808 commented Aug 12, 2016

squaresurf commented Aug 12, 2016

squaresurf commented Aug 12, 2016

coveralls commented Aug 12, 2016

squaresurf commented Aug 12, 2016

justin808 commented Aug 14, 2016

alex35mil commented Aug 15, 2016

squaresurf commented Aug 15, 2016

robwise commented Aug 16, 2016

justin808 commented Aug 16, 2016

robwise commented Aug 16, 2016 • edited Loading

squaresurf commented Aug 16, 2016

justin808 commented Aug 16, 2016

Change lines:

Full code

squaresurf commented Aug 16, 2016

justin808 commented Aug 16, 2016

squaresurf commented Aug 16, 2016

justin808 commented Aug 17, 2016

dzirtusss commented Aug 17, 2016

dzirtusss commented Aug 17, 2016

dzirtusss commented Aug 17, 2016

justin808 commented Aug 17, 2016

squaresurf commented Aug 17, 2016

justin808 commented Aug 17, 2016

robwise commented Aug 17, 2016

justin808 commented Aug 18, 2016

robwise commented Aug 18, 2016

justin808 commented Aug 18, 2016

robwise commented Aug 18, 2016

justin808 commented Aug 18, 2016

robwise commented Aug 18, 2016

justin808 commented Aug 18, 2016

justin808 commented Aug 20, 2016

squaresurf commented Aug 22, 2016

squaresurf commented Jul 21, 2016 •

edited by justin808

Loading

robwise commented Aug 16, 2016 •

edited

Loading