Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

feat(debugger): make element explorer work with node 0.12.0 #1898

Closed
wants to merge 1 commit into from

Conversation

hankduan
Copy link
Contributor

@hankduan hankduan commented Mar 4, 2015

Node has changed its debugger significantly in 0.12.0, and these changes are necessary to get it to work now.

Specifically here are the key points to take note:

  • Before, the process continues running after process._debugProcess(pid); is called, but now, the process stops.
    To over come this, the call to process._debugProcess(pid) is moved from protractor into the debugger client. Secondly, because the process stops after the call, we call reqContinue once the debugger connection is established, so that protractor continues into the first break point (for backwards compatibility, we added an extra empty webdriver command so that in earlier versions of node, protractor doesn't go past the first break point from the reqContinue).
  • Before repl provides '(foobar\n)' when an user inputs 'foobar'. Now it is just 'foobar\n'.
    We will parse and strip away both the parenthesis and '\n' to support all versions of node.

Additionally, this change makes debugger processes fail fast if the port is taken.

@hankduan hankduan force-pushed the nodeissue branch 2 times, most recently from 0625426 to 1d40a24 Compare March 4, 2015 21:55
}, function() {});
});

process.debugPort = opt_port || 5858
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a trailing ';' here.

@hankduan
Copy link
Contributor Author

hankduan commented Mar 4, 2015

oops, forgot to run through linter. Made some style changes thanks @tullmann

@floribon
Copy link

That indeed unlocked the elementExplorer (and browser.pause) for me.
However, commands take a while and freeze most of the time. I'm using iojs@1.4.3 which forked node 0.10.35

@hankduan
Copy link
Contributor Author

Node versions above 0.10.30 is affected by the nodejs bug described here: #1890 (i.e. elementexplorer/pause works but suffer delays)

@floribon
Copy link

Yes but I was experiencing a different bug, the debugger wouldn't work at all, it was impossible to enter any command. With these commits I fall back to that sluggish bug indeed. Using protractor 1.6.1 didn't work for me either (may do with these commits if compatible though), so just saying for any potential iojs user. Thanks for your work.

@hankduan hankduan force-pushed the nodeissue branch 3 times, most recently from 2bd6f14 to 85e18f0 Compare March 10, 2015 08:01
@hankduan
Copy link
Contributor Author

@juliemr when you review this, please take note of how I check the port for whether it is in use or not, as I'm not sure if that's the best way to do it.

@hankduan hankduan force-pushed the nodeissue branch 2 times, most recently from 252d7d3 to fd05ef1 Compare March 10, 2015 09:04
@juliemr juliemr added this to the Pending milestone Mar 10, 2015
@hankduan hankduan force-pushed the nodeissue branch 5 times, most recently from 544d59e to 1709e62 Compare March 26, 2015 00:30
@hankduan hankduan changed the title experimental(debugger): make element explorer work with node 0.12.0 feat(debugger): make element explorer work with node 0.12.0 Mar 26, 2015
Node has changed its debugger significantly in 0.12.0, and these changes are
necessary to get it to work now.

Specifically here are the key points to take note:

* Before, the process continues running after `process._debugProcess(pid);` is
called, but now, the process stops.
> To over come this, the call to `process._debugProcess(pid)` is moved from
protractor into the debugger client. Secondly, because the process stops after
the call, we call `reqContinue` once the debugger connection is established, so
that protractor continues into the first break point (for backwards
compatibility, we added an extra empty webdriver command so that in earlier
versions of node, protractor doesn't go past the first break point from the
reqContinue).

* Before repl provides '(foobar\n)' when an user inputs 'foobar'. Now it is
just 'foobar\n'.
> We will parse and strip away both the parenthesis and '\n' to support all
versions of node.

* Additionally (non-related to node 0.12.0), this change makes debugger
processes fail fast if the port is taken.
@sibelius
Copy link

+1

1 similar comment
@bootstraponline
Copy link

👍

@juliemr juliemr self-assigned this May 15, 2015
@juliemr juliemr modified the milestones: 2.1, Pending May 15, 2015
@@ -103,6 +99,11 @@ WdDebugger.prototype.initRepl_ = function() {

// We want the prompt to show up only after the controlflow text prints.
this.dbgRepl.printControlFlow_(function() {
// Backward compatibility: node version 0.8.14 has a number of built in
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to worry about versions <10 anymore, feel free to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this, as discussed offline.

@juliemr
Copy link
Member

juliemr commented May 15, 2015

Looks good, just a couple questions to make sure I understand the changes.

@hankduan
Copy link
Contributor Author

merged: de49969

@hankduan hankduan closed this May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants