Skip to content

Add a pathStrip option to the react native plugin - #515

Merged
benvinegar merged 1 commit into
getsentry:masterfrom
nevir:rn-prefix
Mar 16, 2016
Merged

benvinegar merged 1 commit into
getsentry:masterfrom
nevir:rn-prefix

Conversation

@nevir

@nevir nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

This allows for the user to customize how raven normalizes file URIs. For example, to support a Code Push bundle in addition to the baked in one:

Raven.addPlugin(require('raven-js/plugins/react-native'), {
  // Strip all paths up to the last /, resulting in paths like
  // `/main.jsbundle`, `/app.jsbundle`, etc.
  pathStrip: /^.*(?=\/)/,
});

@benvinegar

Copy link
Copy Markdown
Contributor

Cool. My only comment is: can we come up with a better name than preventDefault. That name is so synonymous with DOM events that I'd prefer we avoided it.

Some quick ideas:

  • { rethrow: true }
  • { useDefaultExceptionHandler: true }

@nevir

nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

rethrow is a good'un

Should/can I change default behavior to not rethrow as part of this as well?

@benvinegar

Copy link
Copy Markdown
Contributor

Maybe. Another possibility (to fix #468) is we verify the request is sent before calling the original error handler. I'm not sure if that's a good idea or not though – e.g. if the crash takes ~500ms to actually appear / manifest in the app.

@nevir

nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

Alright, I'll stick with just the option for now, that can be layered on later

@benvinegar

Copy link
Copy Markdown
Contributor

Well, I like rethrow if the default behavior is not to rethrow :) So maybe your suggestion (flipping the default back) is fine. I just need another opinion from a React Native developer to chime in here.

@nevir

nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

Well, I can do the options.rethrow === false approach (e.g. rethrow: true being the default) if the name sticks :P

@nevir

nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

Here's the easy way out of noRethrow

@nevir

nevir commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

Aha, ok. So, after further testing, iOS react's dev mode does use the global exception handler to display error redboxes.

It seems like the better behavior here is for default behavior to rethrow in dev mode (for redbox), and not to rethrow otherwise (to prevent the exception from bubbling out). That preserves the behavior of raven-js 1.x, at least


Pushed an update with that behavior

@benvinegar

Copy link
Copy Markdown
Contributor

@nevir – Hey, just a heads up I was out for vacation but I'm back to look at this now.

I just want to fire up this patch and verify the behavior personally before accepting. Probably by EOD tomorrow.

Comment thread plugins/react-native.js Outdated

var defaultHandler = ErrorUtils.getGlobalHandler && ErrorUtils.getGlobalHandler() || ErrorUtils._globalHandler;

var rethrow = pluginOptions.rethrow || __DEV__;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If rethrow is false, this will still throw in __DEV__.

How about:

var rethrow = 'rethrow' in pluginOptions ? pluginOptions.rethrow || __DEV__;

@benvinegar

Copy link
Copy Markdown
Contributor

@nevir – btw, now that we've figured out what the default behavior should be (with __DEV__), is there still a need for the rethrow option?

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Probably not! updating to cut that out

This allows for the user to customize how raven normalizes file URIs.  For example, to support a Code Push bundle in addition to the baked in one:

```js
Raven.addPlugin(require('raven-js/plugins/react-native'), {
  // Strip all paths up to the last /, resulting in paths like
  // `/main.jsbundle`, `/app.jsbundle`, etc.
  pathStrip: /^.*(?=\/)/,
});
```
@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Updated - PTAL

@nevir nevir changed the title Additional options for the react native plugin Add a pathStrip option to the react native plugin Mar 16, 2016
@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

I'm unsure how to appropriately test the pathSplit option without a pretty substantial refactor (or only testing _normalizeData in isolation, which misses most of it) - so skipped a test there :S

@benvinegar

Copy link
Copy Markdown
Contributor

@nevir – first off, thanks for sticking with this. Will definitely merge in.

One last question re: __DEV__ and the global exception handler: if Raven.js is absent in a React Native app, and an exception is thrown, does it not call the global handler / crash the app?

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Uncaught JS exceptions do crash the app by default (i.e. including Raven prevents that)

want me to bring the option back? :P

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

The problem is that if the app is allowed to crash, the exception will be never caught.

Or Raven could defer the default handler until the exception is reported, but you risk additional state corruption; there's no way to pause the rest of the VM

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

(A better solution might be to have react-native support in the ObjC and Java clients; but that sounds thorny)

@benvinegar

Copy link
Copy Markdown
Contributor

If you don't mind, let's pull out your __DEV__/global exception handler changes and put that in another PR where we can continue this conversation. I don't want to hang up your pathStrip changes, which I think are separate.

(If this is a pain in the ass I can do it for you.)

@benvinegar

Copy link
Copy Markdown
Contributor

I understand the predicament where the app crashes before the HTTP request can go out ... but I'm not sure we should just swallow the exception entirely. Maybe waiting for the HTTP response (w/ a short timeout) is best (e.g. rethrow in .5 secs).

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Sounds good, updated to just that commit

benvinegar added a commit that referenced this pull request Mar 16, 2016
Add a `pathStrip` option to the react native plugin
@benvinegar
benvinegar merged commit 6aff89c into getsentry:master Mar 16, 2016
@nevir
nevir deleted the rn-prefix branch March 16, 2016 22:11
@benvinegar

Copy link
Copy Markdown
Contributor

👍

@nevir

nevir commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Added #533 to kickstart the conversation

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