-
Notifications
You must be signed in to change notification settings - Fork 51
Add ScopeMirror traversal to state.js #142
Add ScopeMirror traversal to state.js #142
Conversation
@matthewloring @ofrobots PTAL |
d776780
to
48781ed
Compare
@ofrobots @matthewloring CI is now green and ready for another look |
Could this PR include tests for the case where nested variables have the same name? Also, is this the change that resolves error objects defined in catch blocks? |
48781ed
to
ce97861
Compare
@matthewloring updated |
Let's look into the 0.12/4.0 behavioral difference from the failing test. |
b8dad03
to
065a8f8
Compare
@@ -587,8 +601,13 @@ describe('v8debugapi', function() { | |||
|
|||
var topFrame = bp.stackFrames[0]; | |||
assert.equal(topFrame['function'], 'foo'); | |||
assert.equal(topFrame.arguments[0].name, 'n'); | |||
assert.equal(topFrame.arguments[0].value, '3'); | |||
if (semver.satisfies(process.version, '<1.6')) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@ofrobots @matthewloring any traction on this? |
/*4*/ var a = 10; | ||
/*5*/ a += 1; | ||
/*6*/ return (function (b) { | ||
/*7*/ var a = 10; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
LGTM w/ nit on my end. We should have ofrobots@ take a look at the version dependencies introduced in the tests. |
varTableIndex: ARG_LOCAL_LIMIT_MESSAGE_INDEX | ||
}]; | ||
} else { | ||
args = [{ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Could we add additional tests for each of the internal bugs addressed in this issue. |
c90c24f
to
72e780d
Compare
@matthewloring @ofrobots Feedback integrated; looks like Travis is having troubles right now and the builds are just sitting there. Tested locally with Node.JS 0.12 and 6.3.1 |
if (argMatch && (semver.satisfies(process.version, '<1.6'))) { | ||
// If the version is lower than 1.6 we will use the frame's argument | ||
// list to source argument values, yet the ScopeMirror traversal for | ||
// node these Node versions will also return the arguments. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
/*7*/ }; | ||
/*8*/ return a(1); | ||
/*9*/} | ||
/*10*/module.exports = foo; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
LGTM with a few nits. Let's get a final sign off from @ofrobots. |
8c72049
to
ea5fc89
Compare
@matthewloring @ofrobots Updated |
ea5fc89
to
59d060d
Compare
@@ -0,0 +1,11 @@ | |||
/*1* KEEP THIS CODE AT THE TOP TO AVOID LINE NUMBER CHANGES */ /* jshint shadow:true */ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
lgtm once comments are addressed |
ea8d60d
to
3fa14c6
Compare
Replace the `FrameMirror.localValue` code path with `ScopeMirror` traversal and rewrite the arguments collector to only serialize arguments which are not matched by name with a more deeply-scoped value. Add closure scope type to filter list because of confusion with node.JS requires. Add tests for nested, confounding variable names and try/catch. 03/08/2016-10:07 PST Update to use locals only becuase of 0.12 behaviour. 12/08/2016-12:40 PST Update with full-range of test-cases gathered from bug-bash. Add more testing around locals for node versions less than 1.6. Add more documentation around new ScopeMirror traversal. Add 'this' context extraction and tests. 15/08/2016-10:54 PST Respond to PR feedback
@ofrobots @matthewloring Updated. I don't have the power to merge in though |
You have the power now. |
Replace the
FrameMirror.localValue
code path withScopeMirror
traversal and rewrite the arguments collector to only serialize
arguments which are not matched by name with a more deeply-scoped
value.