Fix stacktraces for overwritten properties and methods #661
Conversation
|
@keithamus any comments on this? |
| @@ -79,7 +78,7 @@ module.exports = function (ctx, name, method, chainingBehavior) { | |||
|
|
|||
| var assert = function assert() { | |||
| var old_ssfi = flag(this, 'ssfi'); | |||
| if (old_ssfi && config.includeStack === false) | |||
| if (old_ssfi) | |||
There was a problem hiding this comment.
Could you please explain why you've removed the config.includeStack part here?
There was a problem hiding this comment.
That flag is already checked in the assert() method and checking it here has no real benefit aside from making the logic more complicated
|
LGTM, I'd like @lucasfcosta to review this one also though. |
|
Sure, it needs a bit of brain twisting to understand what's going on 😉 |
| if (!keep_ssfi && old_ssfi) | ||
| flag(this, 'ssfi', ctx[name]); | ||
|
|
||
| flag(this, 'keep_ssfi', true); |
There was a problem hiding this comment.
Hi @Turbo87, I just want to confirm I understood the whole thing here:
Whenever you overwrite a method you set keep_ssfi to true before calling _super so you won't get that call on the stack due to those checks you added to addMethod, addChainableMethod and addProperty, am I right?
|
Hello everyone, first of all, thanks for the PR @Turbo87, great job here 😄 ! I needed to pay a lot of attention to really understand how to handle the Stack Trace properly, but I think I've got it. This is looking good in general, anyway, I just added two notes to the code just to ensure we're going in the right direction here (and if I've understood everything correctly). |
config.includeStack is already checked by the assert() function
|
@lucasfcosta I've added a few tests to verify that the stack traces work correctly when overwriting properties and methods |
| // Phantom does not include function names for getter exec | ||
| if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) { | ||
| expect(err.stack).to.include('utilities.js'); | ||
| expect(err.stack).to.not.include('overwriteProperty'); |
There was a problem hiding this comment.
I believe you should be checking if it does not include overwriteMethod instead of overwriteProperty here, shouldn't you?
There was a problem hiding this comment.
hahaha, np, happens to everyone sometimes 😆
|
Great job @Turbo87, I just think you accidentally looked for |
|
@lucasfcosta fixed |
|
This LGTM! |
This PR resolves #659
Since testing overwritten properties/methods doesn't seems to be possible at the moment I tested this manually and it seems to work now. It would be good if this could be verified by someone else before merging though!
/cc @keithamus