Refactor Pending Breakpoints to fix reloading #2877
Conversation
src/actions/sources.js
Outdated
|
|
||
| for (const source in filteredSources) { | ||
| await dispatch(newSource(source)); | ||
| } |
There was a problem hiding this comment.
we need to update this before we merge!
codehag
changed the title
| const bp = getBreakpoint(state, location); | ||
|
|
||
| if (sameSource && !bp) { | ||
| if (location.column && isEnabled("columnBreakpoints")) { |
There was a problem hiding this comment.
As mentioned earlier, this block probably needs to be refactored. The if statements do not make sense, as in all cases we will call await dispatch(addBreakpoint(location, { condition }));. Instead we probably want to use an early return if !isEnabled("columnBreakpoints").
src/actions/sources.js
Outdated
| } | ||
| }); | ||
| const pendingBreakpointsList = pendingBreakpoints.valueSeq().toJS(); | ||
| for (let pendingBreakpoint of pendingBreakpointsList) { |
src/actions/sources.js
Outdated
| } | ||
| } | ||
| }); | ||
| const pendingBreakpointsList = pendingBreakpoints.valueSeq().toJS(); |
There was a problem hiding this comment.
nitpick: list has a lot of different connotations. for more precision could we perhaps call it pendingBreakpointsArray? I think it might be more meaningful to newcomers
src/actions/tests/breakpoints.js
Outdated
| await dispatch(actions.addBreakpoint({ sourceId: "a", line: 5 })); | ||
| await dispatch(actions.addBreakpoint({ sourceId: "b", line: 6 })); | ||
| it("when the user adds a sliding breakpoint, a corresponding pending breakpoint should be added", async () => { | ||
| const { dispatch, getState } = createStore(slidingMockThreadClient); |
There was a problem hiding this comment.
nitpick: I think it might be easy to break this, by not getting the right offset between slidingMockThreadClient and slidLocation. We can explicitly associate these two (or more) stub elements by using a wrapper function. Here is an example of how it might be done:
function generateMockThreadClient (offset = 0) {
return {
setBreakpoint: (location, condition) => {
return new Promise((resolve, reject) => {
const actualLocation = Object.assign({}, location, {
line: location.line + offset
});
resolve({ id: makeLocationId(location), actualLocation, condition });
});
}
};
};
function generateOffsetThreadClient(offset, location) => {
const slidingMockThreadClient = generateSlidingMockThreadClient(offset);
const slidLocation = Object.assign({}, location, { line: location.line + offset });
return { slidingMockThreadClient, slidLocation };
}
function ... () { // wherever this will be used
const location = { sourceId: "a", line: 5 };
const { slidingMockThreadClient, slidLocation } = generateOffsetThreadClient(2, location);
const { dispatch, getState } = createStore(slidingMockThreadClient);
... // and so on
}
naming needs to be improved. it might make sense to make location an array, but i think its getting ahead of ourselves
| }; | ||
| } | ||
|
|
||
| function slideMockBp(bp) { |
There was a problem hiding this comment.
same comment here as above on line 79 in breakpoints.js --- probably we can do this in a future pr, where we create a breakpoints utils
|
|
||
| break; | ||
| const newState = setCondition(state, action); | ||
| setPendingBreakpoints(newState); |
src/reducers/breakpoints.js
Outdated
| const pendingId = makePendingLocationId(action.breakpoint.location); | ||
| let updatedState = undefined; | ||
|
|
||
| if (action.disabled) { |
There was a problem hiding this comment.
since these two cases are so different, perhaps they can be functions that return updatedState? then we won't need to do the weird let
src/reducers/breakpoints.js
Outdated
|
|
||
| const locationId = makeLocationId(breakpoint.location); | ||
| const movedLocationId = makeLocationId(actualLocation); | ||
| state = state.deleteIn(["breakpoints", locationId]); |
There was a problem hiding this comment.
Personal comment: Its not wrong, I just cringe if I see state being reassigned :P
src/reducers/breakpoints.js
Outdated
| function filterByNotLoading(bp: any): boolean { | ||
| return !bp.loading; | ||
| function setPendingBreakpoints(state) { | ||
| prefs.pendingBreakpoints = state.pendingBreakpoints; |
| } | ||
|
|
||
| function restorePendingBreakpoints() { | ||
| return prefs.pendingBreakpoints; | ||
| return I.Map(prefs.pendingBreakpoints); |
Codecov Report
@@ Coverage Diff @@
## master #2877 +/- ##
=========================================
+ Coverage 58.63% 60% +1.36%
=========================================
Files 63 63
Lines 2403 2430 +27
Branches 493 496 +3
=========================================
+ Hits 1409 1458 +49
+ Misses 994 972 -22
Continue to review full report at Codecov.
|
codehag
changed the title
Associated Issue: #2817
Summary of Changes