(WIP) call sites#3240
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3240 +/- ##
=========================================
- Coverage 49.35% 48.5% -0.85%
=========================================
Files 99 100 +1
Lines 4156 4251 +95
Branches 856 876 +20
=========================================
+ Hits 2051 2062 +11
- Misses 2105 2189 +84
Continue to review full report at Codecov.
|
jasonLaster
left a comment
There was a problem hiding this comment.
some comments, really close
| editor: this.editor && this.editor.codeMirror | ||
| }) | ||
| ); | ||
| );*/ |
There was a problem hiding this comment.
we should remove the column breakpoints code here and the component
| } | ||
|
|
||
| renderCallSites() { | ||
| const editor = this.editor; |
There was a problem hiding this comment.
lets add the column bps feature flag here
|
|
||
| this.setState({ showCallSites: false }); | ||
| } | ||
| } |
There was a problem hiding this comment.
lets move onKeyUp and onKeyDown to CallSites because we can :)
| let { key } = e; | ||
|
|
||
| if (key === "Alt") { | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
others might handle this too so perhaps we dont want to stop propogation
|
|
||
| .call-sites { | ||
| outline: 1px solid #aed3ef; | ||
| } |
There was a problem hiding this comment.
this should be in its own call-sites css file
| const sourceId = selectedLocation && selectedLocation.sourceId; | ||
| const source = selectedSource && selectedSource.toJS(); | ||
|
|
||
| getCallSiteBreakpoint = location => { |
There was a problem hiding this comment.
this should be in its own helper
| callSite => callSite.location.start.line === callSite.location.end.line | ||
| ); | ||
|
|
||
| editor.codeMirror.operation(() => { |
There was a problem hiding this comment.
i'm not sure if this helped... lets look at the perf here. Maybe not a blocker, but nice
There was a problem hiding this comment.
i tot it did when we tested it, at the allhands ... i'll try it again
|
|
||
| if (t.isCallExpression(path)) { | ||
| const callee = path.node.callee; | ||
| const calleeNode = t.isMemberExpression(callee) |
There was a problem hiding this comment.
we should exclude member expressions cause... well you know
| this.clearCallSite(); | ||
| } | ||
| } | ||
| // console.log(nextProps.showCallSite); |
jasonLaster
left a comment
There was a problem hiding this comment.
Looks great! one question about the snapshot though
| [(2, 8), (2, 9)] [(2, 0), (2, 9)] c | ||
| [(2, 4), (2, 5)] [(2, 0), (2, 5)] b | ||
| call expressions | ||
| undefined |
There was a problem hiding this comment.
it looks like we might be getting the member expressions and not the call expressions with this change?
| key: index, | ||
| callSite, | ||
| editor, | ||
| breakpoint: callSite.breakpoint, |
There was a problem hiding this comment.
we might not need to pull this in on its own since we have it on callSite
|
I made some progress on this today, but ultimately could not get it all the way there: done:
not done:
|
|
looking into the failure on removing breakpoints might have an idea what the issue is ... |
Opening an early PR, which we can collaborate on.