Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

switch bpclient ids to location ids#3163

Merged
jasonLaster merged 8 commits into
firefox-devtools:masterfrom
jasonLaster:bp-ids
Jun 23, 2017
Merged

switch bpclient ids to location ids#3163
jasonLaster merged 8 commits into
firefox-devtools:masterfrom
jasonLaster:bp-ids

Conversation

@jasonLaster
Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster commented Jun 13, 2017 •
edited by bomsy
Loading

Associated Issue: #3182

Summary of Changes

  • I ran into an issue debugging devtools-reporter. The issue was that on reload the BP id had changed, but the source id and location were the same... Which got me thinking we should be consistent and use location ids
  • I also set about adding some integration tests and cleaning them up. I'll do a mochitest for this as well before it lands

codehag
codehag reviewed Jun 14, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@codehag codehag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nice changes!

Comment thread src/client/firefox/commands.js Outdated
const id = makeLocationId(location);
const bpClient = bpClients[id];

// const values = _.values(bpClients);
Copy link
Copy Markdown
Contributor

@codehag codehag Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can just delete this?

Comment thread src/client/firefox/commands.js
location.condition === condition
);
});
const id = makeLocationId(location);
Copy link
Copy Markdown
Contributor

@codehag codehag Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks much better!

Comment thread src/client/firefox/commands.js
})
.then((res: BreakpointResponse) => onNewBreakpoint(location, res));
.then(([{ actualLocation }, bpClient]) => {
actualLocation = createBreakpointLocation(location, actualLocation);
Copy link
Copy Markdown
Contributor

@codehag codehag Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't wrong in terms of logic, but I find it difficult to follow if args are reassigned in a function. Generally, I would avoid reassignment due to how it impacts code reading. In this case, it's ok because we don't have if statements, but as a preventative measure, I think it would be better to use a new variable here. In the lower case where we are reassigning bpClients[i], it's unavoidable, so that can stay as it is.

Comment thread src/client/firefox/create.js
// but we want it always to exist. Format `actualLocation` if it
// exists, otherwise use `location`.

export function createBreakpointLocation(
Copy link
Copy Markdown
Contributor

@codehag codehag Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 great

Comment thread src/client/firefox/types.js
source: SourceClient
};

export type BPClients = { [id: ActorId]: BreakpointClient };
Copy link
Copy Markdown
Contributor

@codehag codehag Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 great

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 15, 2017 •
edited
Loading

Codecov Report

Merging #3163 into master will not change coverage.
The diff coverage is 8.69%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3163   +/-   ##
======================================
  Coverage    47.9%   47.9%           
======================================
  Files          98      98           
  Lines        4066    4066           
  Branches      839     835    -4     
======================================
  Hits         1948    1948           
  Misses       2118    2118
Impacted Files Coverage Δ
src/client/firefox/commands.js 15.74% <0%> (+0.56%) ⬆️
src/client/firefox/create.js 13.33% <0%> (-4.85%) ⬇️
src/reducers/breakpoints.js 90.19% <100%> (ø) ⬆️
src/actions/breakpoints.js 87.62% <100%> (ø) ⬆️

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 9b77377...dd97e26. Read the comment docs.

@codehag codehag changed the title [wip] switch bpclient ids to location ids switch bpclient ids to location ids Jun 22, 2017
codehag
codehag approved these changes Jun 23, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@codehag codehag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasonLaster jasonLaster merged commit f8ee691 into firefox-devtools:master Jun 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@codehag codehag codehag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jasonLaster @codehag