Refactor Breakpoints to be location symmetric by jasonLaster · Pull Request #3294 · firefox-devtools/debugger

jasonLaster · 2017-07-10T14:43:03Z

Summary of Changes

This is a follow up refactor. The goal is to provide some abstractions so that BPs are better represented in the components:

Components don't know/nor care if they're original or generated
Components only care about showing BPs
Toggle BP takes a line and column and does the rest
The reducer formats a generic BP with the "right" location

codehag · 2017-07-10T14:49:46Z

+    const { sourceId } = selectedLocation;
+
+    if (bp) {
+      // NOTE: it's possible the breakpoint has slid to a column


you can use the equalize location util!

codehag · 2017-07-10T14:50:40Z

+  };
+}
+
+export function toggleBreakpointDisabledStatus(line) {


i like this, might need a simpler name like "toggleDisabledBreakpoint"

codehag

nice direction!

codehag · 2017-07-10T14:51:05Z

+
+    const { sourceId } = selectedLocation;
+
+    if (!bp) {


i wonder if we can avoid this somehow... 🤔

codehag · 2017-07-10T14:52:23Z

    return !!this.cbPanel;
  }

-  toggleBreakpoint(line, column = undefined) {


👍 niiice

codehag · 2017-07-10T14:52:41Z

    });
  }

-  renderBreakpoints() {


codecov · 2017-07-11T18:24:08Z

Codecov Report

Merging #3294 into master will increase coverage by 0.46%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #3294      +/-   ##
==========================================
+ Coverage   48.46%   48.92%   +0.46%     
==========================================
  Files         102      105       +3     
  Lines        4288     4319      +31     
  Branches      891      893       +2     
==========================================
+ Hits         2078     2113      +35     
+ Misses       2210     2206       -4

Impacted Files	Coverage Δ
src/selectors.js	`100% <ø> (ø)`	⬆️
src/components/Editor/CallSites.js	`2.12% <ø> (ø)`	⬆️
src/selectors/linesInScope.js	`93.75% <ø> (ø)`	⬆️
src/utils/editor/index.js	`14.63% <0%> (-0.76%)`	⬇️
src/components/Editor/GutterMenu.js	`0% <0%> (ø)`	⬆️
src/utils/editor/expression.js	`0% <0%> (ø)`	⬆️
src/selectors/visibleBreakpoints.js	`0% <0%> (ø)`
src/components/Editor/Breakpoint.js	`6.38% <0%> (+0.92%)`	⬆️
src/actions/utils/breakpoints.js	`78.26% <100%> (-13.05%)`	⬇️
src/components/Editor/Breakpoints.js	`13.33% <13.33%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dc9f00...2df507c. Read the comment docs.

jasonLaster · 2017-07-12T03:00:06Z

    const action = {
      type: "DISABLE_BREAKPOINT",
      breakpoint: bp,
-      disabled: true,


yup, not being used...

jasonLaster · 2017-07-12T03:00:29Z

+    source: "function foo2(x, y) {\n  return x + y;\n}",
+    contentType: "text/javascript"
+  }
+};


perhaps there's a better way to keep this dummy data...

could it make sense to start using a factory library?

yeah - that might be a nice cleanup in a separate PR.

jasonLaster · 2017-07-12T03:01:34Z

+    selectedSource: getSelectedSource(state)
+  }),
+  dispatch => bindActionCreators(actions, dispatch)
+)(Breakpoints);


I like the pattern of splitting out the editor children components. The separation makes it easier to follow

@asolove @codehag

jasonLaster · 2017-07-12T03:02:13Z

+    const line = breakpoint.location.line - 1;

-    editor.setGutterMarker(
+    showSourceText(editor, selectedSource.toJS());


This is important for making sure that we're updating the correct codemirror document.

jasonLaster · 2017-07-12T03:08:47Z

+}
+
+/*
+ *


I should include a quick description

jasonLaster · 2017-07-12T03:09:06Z

+    : breakpoint.location;
+}
+
+export default function getVisibleBreakpoints(state: OuterState) {


There should be a quick comment to explain how this is used

is this so that we can check if a breakpoint is also represented in a generated file? can we use it in all cases?

This will get the breakpoints for a file at the "correct" location regardless of original / generated

jasonLaster · 2017-07-12T03:09:28Z

  is(getBreakpoints(getState()).size, 1, "One breakpoint exists");
  ok(
-    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4 }),
+    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4, column: 2 }),


not sure why this changed :/

It is probably alright though

this might be a regression - before we were making sure that we didn't turn line breakpoints into column breakpoints...

jasonLaster · 2017-07-12T03:09:42Z

  return (
-    location.line !== newLocation.line ||
-    (location.column != null && location.column !== newLocation.column)
+    location.line !== newLocation.line || location.column !== newLocation.column


this is a nice clean up

jasonLaster · 2017-07-12T03:09:56Z

-      : breakpoint.location;
-
-    const sameLine = location.line === line + 1;
+    const sameLine = breakpoint.location.line === line + 1;


i love these cleanups.

codehag

Great work so far!

codehag · 2017-07-12T09:07:59Z

+    }
+
+    if (bp) {
+      // NOTE: it's possible the breakpoint has slid to a column


do we want to slide a breakpoint to a column if the user is expecting a line breakpoint?

codehag · 2017-07-12T09:08:27Z

+    source: "function foo2(x, y) {\n  return x + y;\n}",
+    contentType: "text/javascript"
+  }
+};


could it make sense to start using a factory library?

codehag · 2017-07-12T09:33:11Z

+      return null;
+    }
+
+    return dom.div(


the Breakpoint component returns null; so, do we need to wrap it in a div, or just return null, with the mapping done in the body of the function?

codehag · 2017-07-12T09:34:15Z


 export default function GutterMenu({
-  bp,
+  breakpoint,


codehag · 2017-07-12T09:38:45Z

+  return isGeneratedId(sourceId);
+}
+
+function getColumn(column, selectedSource) {


I like this pattern better than keeping the selectors in the reducer. maybe we could have a folder structure here:

selectors/ |- breakpoints/ |- index.js |- atLocation.js

codehag · 2017-07-12T09:40:04Z

+    : breakpoint.location;
+}
+
+export default function getVisibleBreakpoints(state: OuterState) {


is this so that we can check if a breakpoint is also represented in a generated file? can we use it in all cases?

codehag · 2017-07-12T09:40:34Z

  is(getBreakpoints(getState()).size, 1, "One breakpoint exists");
  ok(
-    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4 }),
+    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4, column: 2 }),


this might be a regression - before we were making sure that we didn't turn line breakpoints into column breakpoints...

codehag · 2017-07-12T09:41:00Z

-      : breakpoint.location;
-
-    const sameLine = location.line === line + 1;
+    const sameLine = breakpoint.location.line === line + 1;


codehag reviewed Jul 10, 2017

View reviewed changes

jasonLaster mentioned this pull request Jul 11, 2017

Cannot set debugger breakpoint on generatedSource #3167

Closed

jasonLaster added this to the July 25th milestone Jul 11, 2017

jasonLaster changed the title ~~Refactor Breakpoints~~ Refactor Breakpoints to be location symmetric Jul 11, 2017

jasonLaster force-pushed the refactor-bps2 branch 2 times, most recently from 6eb8bee to b2f407c Compare July 12, 2017 02:59

jasonLaster commented Jul 12, 2017

View reviewed changes

codehag reviewed Jul 12, 2017

View reviewed changes

codehag approved these changes Jul 12, 2017

View reviewed changes

jasonLaster force-pushed the refactor-bps2 branch from b2f407c to 28a8e09 Compare July 12, 2017 15:11

Refactor toggling breakpoints logic

2df507c

jasonLaster force-pushed the refactor-bps2 branch from 28a8e09 to 2df507c Compare July 12, 2017 16:18

jasonLaster merged commit d66e4c0 into firefox-devtools:master Jul 12, 2017

codehag mentioned this pull request Nov 3, 2017

Improve JSX syntax highlighting #4539

Merged

+              }
+              /*
+               *

Uh oh!

Conversation

jasonLaster commented Jul 10, 2017

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 11, 2017 •

edited

Loading