Implements getScope parsing in the devtools-map-bindings #656
Conversation
jasonLaster
left a comment
jasonLaster
left a comment
There was a problem hiding this comment.
Looking good. A couple of style comments.
| const { sourceId } = location; | ||
| const visitor = createParseJSScopeVisitor(sourceId); | ||
| traverseAst(source, visitor.traverseVisitor); | ||
| const parsedScopes = visitor.toParsedScopes(); |
There was a problem hiding this comment.
I like how you pull in the test traverseAst helper
| } | ||
| tree.params.forEach(param => { | ||
| parseDeclarator(param, scope, "param"); | ||
| }); |
There was a problem hiding this comment.
nit:
tree.params.forEach(param => parseDeclarator(param, scope, "param")))
| }); | ||
| } else if (isNode(declaratorId, "RestElement")) { | ||
| parseDeclarator(declaratorId.argument, targetScope, type); | ||
| } |
There was a problem hiding this comment.
we'll want some tests for these cases
| let zero = 0; | ||
| function fn(a, b) { | ||
| for (let i of [1, 2]) { | ||
| const i = a + i + zero; |
There was a problem hiding this comment.
it'd be nice if i were index so that we can have another minifier check
| // Find nearest outer scope with the specifed name and add reference. | ||
| for (let s = scope; s; s = s.parent) { | ||
| if (name in s.names) { | ||
| s.names[name].refs.push(loc.start); |
There was a problem hiding this comment.
it would be nice if this function avoided mutating scope and instead returned the found location:
| return; | ||
| } | ||
| if (path.isVariableDeclaration()) { | ||
| const hoistAt = |
There was a problem hiding this comment.
neat. I imagine this might be one of the more interesting considerations because babel is free to add other hoisting rules in the future:
perhaps we want to try looking for the ref:
path.findParent((path) => path.scope.bindings.include(?));NOTE: just guessing based on reading the code, i could be mis-reading this
| if (hasLet) { | ||
| savedParents.set(path, parent); | ||
| parent = createTempScope("Block", parent, location); | ||
| } |
| return false; | ||
| } | ||
| return node.kind === "let" || node.kind === "const"; | ||
| }); |
There was a problem hiding this comment.
nit: can we move this function up top?
function hasLet(path) {
return path.node.body.some(node => {
if (!isNode(node, "VariableDeclaration")) {
return false;
}
return node.kind === "let" || node.kind === "const";
});
}
yurydelendik
force-pushed
the
getScopes2
branch
from
a7c6702 to
3b35eb2
Compare
yurydelendik
force-pushed
the
getScopes2
branch
from
3b35eb2 to
d8bc810
Compare
2 participants
Associated Issue: #624
Summary of Changes