Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XHR breakpoints with methods #7721

Merged
merged 31 commits into from Feb 11, 2019

Conversation

@jaril
Copy link
Contributor

commented Jan 15, 2019

Preview

XHR breakpoints pane with methods
jan-14-2019 19-12-59

Pausing behavior in action
jan-14-2019 19-48-53

Summary of Changes

  • Closes #7645
  • Adds support for XHR breakpoints breaking on specific methods (e.g. GET, POST, etc.)
  • Default is ANY method, which is how all XHR breakpoints are currently set
  • Added tests for XHRBreakpoints component

Test Plan

Run tests with npm run test xhrbreakpoints

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

Wow! Thanks the first pass looks looks great!

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

This looks great @notjaril ! Needs a few updates but very solid! We may also consider a mochitest here as well -- they've been saving us a lot of pain lately.

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

Regardless of edit or new XHR breakpoint, changing the <select>/method resets back to ANY.

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

I believe you'll want to add a selected attribute to the <option> with the matching value

@jaril jaril requested a review from flodolo as a code owner Jan 21, 2019

@jaril

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Regardless of edit or new XHR breakpoint, changing the <select>/method resets back to ANY.

Fixed. Was caused by a specific event behavior on Firefox that re-rendered <select> list upon change, reverting the value to the default "ANY" before the onChange callback was fired.

jan-21-2019 10-46-06

Just need to rewrite mochitests and should be good to go :)

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@notjaril Looks like something bad happened here with a merge; could you rebase?

@jasonLaster jasonLaster force-pushed the jaril:xhr-breakpoints-with-methods branch from 124c2b6 to 9c3c4fe Jan 23, 2019

@darkwing darkwing force-pushed the jaril:xhr-breakpoints-with-methods branch from 9c3c4fe to 1e34f7a Jan 23, 2019

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

@jaril Just one tiny update and I think we should be good here.

@digitarald digitarald requested a review from mcroud Jan 24, 2019

@digitarald

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Summoning @mcroud to give UX feedback.

My preference would be to style the form and the XHR breakpoint entry according to the mockup from the referenced issue:

image

Calling out specifics:

  • Visual difference between method and pattern in the entry (color and type) and using monospaced type for the pattern
  • Layout of the form with separator and dropdown styling aligned with photon dropdowns
@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Perhaps we do a follow up or for styling?

I'm suggesting this because the functionality was a lot of work. Itd be shame for that to go stale while we sort out some of the design aspects.

@digitarald

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Perhaps we do a follow up or for styling?

Yes, my feedback should not block this PR.

@darkwing darkwing force-pushed the jaril:xhr-breakpoints-with-methods branch from 9ce955d to 6f1ab7f Feb 5, 2019

darkwing added some commits Feb 5, 2019

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@jaril The test problem was simply that you needed a rebase. Now there's two small jest test issue, then we should be good to go!

jaril added some commits Feb 5, 2019

Fixed mochitests
Changed implementation of mochitest to get the same results between running tests in TravisCI and locally
@darkwing
Copy link
Contributor

left a comment

Approved once we fix the long URL issue:

99


await dbg.actions.removeXHRBreakpoint(0);
invokeInTab("main", "doc-xhr.html");
await wait(1000); // timeout needed before checking whether it's paused

This comment has been minimized.

Copy link
@darkwing

darkwing Feb 8, 2019

Contributor

Instead of these arbitrary wait times, I think you could add a waitForNotPaused function -- checkout waitForPaused and model it after that.

invokeInTab("main", "doc");
await addXHRBreakpoint(dbg, "doc", "POST");
invokeInTab("main", "doc-xhr.html");
await wait(1000); // timeout needed before checking whether it's paused

This comment has been minimized.

Copy link
@darkwing

darkwing Feb 8, 2019

Contributor

Oops, we'll want a way around these waits; did you try my suggestion re: polling instead of a generic number?

jaril added some commits Feb 8, 2019

this.setState(
// $FlowIgnore
{ inputMethod: e.target.children[1].value },
setXHRBreakpoint

This comment has been minimized.

Copy link
@AnshulMalik

AnshulMalik Feb 10, 2019

Member

Umm, we are setting this setXHRBreakpoint in the state, I don't understand this part.

This comment has been minimized.

Copy link
@jaril

jaril Feb 11, 2019

Author Contributor

We are. This is just a workaround for mochitests to make it testable, since there's no reliable way to simulate a user change event for the <select> element without importing ReactUtils.

Instead, I just programmatically change the value of the <select> element. But changing the value itself doesn't trigger a change event, which doesn't update the inputMethod in state, so I added another "force update" on submit to make sure that it updates the inputMethod on submit.

setState just requests a state change to be queued - it doesn't always immediately update the state as it's called. setXHRBreakpoint is passed into setState specifically to make sure that it happens after the inputMethod is updated on submit. Otherwise, what happens is setXHRBreakpoint gets called first before the state is updated.

This comment has been minimized.

Copy link
@AnshulMalik

AnshulMalik Feb 11, 2019

Member

👍 Makes sense now. Thanks :)

@darkwing darkwing merged commit 4c6f157 into firefox-devtools:master Feb 11, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jasonLaster added a commit that referenced this pull request Feb 12, 2019

@jaril jaril deleted the jaril:xhr-breakpoints-with-methods branch Feb 12, 2019

jasonLaster added a commit that referenced this pull request Feb 12, 2019

@AnshulMalik

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

This is awesome @jaril

@digitarald

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Follow up bug filed for method changing https://bugzilla.mozilla.org/show_bug.cgi?id=1557780

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.