Skip to content

Conversation

leth
Copy link
Contributor

@leth leth commented Feb 5, 2018

Otherwise the original trim value will be off by one.

@leth leth requested a review from kamilogorek as a code owner February 5, 2018 14:28
@kamilogorek kamilogorek changed the title Add to trimHeadFrames if capturing new trace objectMerge Add to trimHeadFrames if capturing new trace Feb 6, 2018
src/raven.js Outdated
stacktrace: true // if we fall back to captureMessage, default to attempting a new trace
},
options
trimHeadFrames: ((options && options.trimHeadFrames) || 0) + 1,
Copy link
Contributor

@kamilogorek kamilogorek Feb 6, 2018

Choose a reason for hiding this comment

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

Can we set the default for options at the top of the method while also making a copy?

options = options ? Object.merge({}, options) : {}

and then just leave (options.trimHeadFrames || 0) + 1?

We also don't need a double objectMerge, as it can be done in a single call when we do so:

objectMerge(options, {
  stacktrace: true,
  trimHeadFrames: (options.trimHeadFrames || 0) + 1
})

Although I'm not sure if we cannot just reverse it, even though it's reference and write

objectMerge(options, {
  stacktrace: true,
  trimHeadFrames: (options.trimHeadFrames || 0) + 1
})

without making a copy, as objectMerge is just reassigning values by key anyway.

Copy link
Contributor Author

@leth leth Feb 7, 2018

Choose a reason for hiding this comment

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

Sounds like a good idea! Thanks!

@kamilogorek
Copy link
Contributor

You missed a comma after stacktrace: true, therefore all tests fail :P

  423:13  error  Parsing error: Unexpected token trimHeadFrames

@kamilogorek
Copy link
Contributor

Fixed it myself and rebase manually. Thanks! :)

@kamilogorek kamilogorek closed this Feb 8, 2018
@leth
Copy link
Contributor Author

leth commented Feb 8, 2018

Sorry, thanks for picking that up!

@leth
Copy link
Contributor Author

leth commented Feb 8, 2018

I can't see this fix on any of the active branches... did it get lost?

@leth leth deleted the patch-1 branch February 8, 2018 14:35
@kamilogorek
Copy link
Contributor

50beaa5 first at the top. I'll make a release within next 20-30min.

@kamilogorek
Copy link
Contributor

Released as 3.22.2

@leth
Copy link
Contributor Author

leth commented Feb 8, 2018

Awesome, thanks!

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