Framework for debug adapter and mock ui #1088

jwli229 · 2017-10-16T19:02:55Z

Release note: none
Issue: #907
Summary:

Mock UI as a CLI to communicate with the adapter
Starter code for the debugger adapter with an initialize request

Test:

Usage: node lib/debugger/mock-ui/debugger-cli.js --adapterPath <path_to_debug_adapter> --prepack <command_to_start_prepack>
e.g. node lib/debugger/mock-ui/debugger-cli.js --adapterPath lib/debugger/adapter/DebugAdapter.js --prepack "lib/prepack-cli.js test/serializer/basic/Date.js"
Order of requests on start-up of UI according to protocol:
1. initialize (UI to adapter): UI tells the adapter to send debugger configuration details (e.g. whether lines should start at 0 or 1)
2. initialize (adapter to UI): Adapter tells the UI to send any persisted debug information (e.g. persisted breakpoints from previous usages of the debugger)
3. configurationDone (UI to adapter): UI tells the adapter that it is done sending persisted debug information
Supported commands thus far:
- init <clientID> <adapterID>: sends an initialize request to the adapter and receives a response printed to console
- configDone: sends a configurationDone request to the adapter and receives a response printed to console
- exit: quit the UI
  Note: for testing purposes, init and configDone are made into user commands. These can be done without user input.

hermanventer · 2017-10-16T21:04:59Z

Your test plan is not reproducible. Ideally, another person should be able to get this pull request and verify that the test plan works on their setup.

You should also strive get have automated tests from day 1.

hermanventer · 2017-10-16T20:31:37Z

src/debugger/adapter/DebugAdapter.js

+  // Start Prepack in a child process
+  _startPrepack() {
+    if (this._prepackCommand.length === 0) {
+      console.error("No command given to start Prepack in adapter");


Is it OK to just carry one in this case?

Terminated the adapter process in this case.

Also note that if no second argument was given, _prepackCommand will be undefined.

hermanventer · 2017-10-16T20:35:18Z

src/debugger/adapter/DebugAdapter.js

+      if (arg.startsWith("--")) {
+        arg = arg.slice(2);
+        if (arg === "prepack") {
+          this._prepackCommand = args.shift();


Does this work if you want to specify arguments to prepack itself?

Yes, the entire command is passed in as one argument. See example above. We can eventually make the command to be read from a file like the launch.json in vscode.

hermanventer · 2017-10-16T20:36:42Z

src/debugger/adapter/DebugAdapter.js

+    });
+
+    this._prepackProcess.on("exit", () => {
+      // Prepack is finished


Should the adapter stay alive in this case?

hermanventer · 2017-10-16T20:40:31Z

src/debugger/adapter/DebugAdapter.js

+  }
+
+  /**
+   * The 'initialize' request is the first request called by the frontend


"frontend"? I assume that is the UI.

hermanventer · 2017-10-16T20:44:36Z

src/debugger/mock-ui/main.js

+import { Session } from "./session.js";
+
+function run(process, console) {
+  let adapterPath: string = "";


No need for a type annotation here. Flow is very good about inferring the types of local variables.

hermanventer · 2017-10-16T21:01:33Z

src/debugger/mock-ui/session.js

+          // reset the content length to ensure it is extracted for the next message
+          this._contentLength = -1;
+          // process the message
+          if (message.length > 0) {


a comment to explain why the message could be length 0 would be helpful.

It should not be, I removed this check.

hermanventer · 2017-10-16T21:03:33Z

src/debugger/mock-ui/session.js

+  }
+
+  _processEvent(event: DebugProtocol.Event) {
+    // to be implemented


Looks like you're not quite done with this.

This and _processEvent will be implemented in the next PR to not make this one too big. For now, they just confirm a message is received and parsed by printing it.

hermanventer · 2017-10-16T21:06:27Z

src/debugger/mock-ui/session.js

+    // they can be done without user input
+    if (command === "init") {
+      let args: DebugProtocol.InitializeRequestArguments = {
+        clientID: "CLI",


Is this a value prescribed by the protocol?

hermanventer · 2017-10-16T21:07:34Z

src/debugger/mock-ui/session.js

+    if (command === "init") {
+      let args: DebugProtocol.InitializeRequestArguments = {
+        clientID: "CLI",
+        adapterID: "Prepack",


Clearly this is not prescribed by the protocol. It does seem a terse. Could there in the future be another adapter that involves Prepack?

hermanventer · 2017-10-16T21:11:26Z

src/debugger/mock-ui/session.js

+//separator for messages according to the protocol
+const TWO_CRLF = "\r\n\r\n";
+
+export class Session {


It is not clear to me what a Session is and why it is distinct from the adapter. Mostly, I'm bemused by the logic that deals with parsing commands and buffering.

I moved the parsing and buffering into a separate class.

jwli229 · 2017-10-17T20:42:53Z

I will set up testing for the debugger in a separate PR afterwards to avoid making this one too big.

cblappert · 2017-10-17T23:28:18Z

src/debugger/adapter/DebugAdapter.js

+  // Start Prepack in a child process
+  _startPrepack() {
+    if (this._prepackCommand.length === 0) {
+      console.error("No command given to start Prepack in adapter");


Also note that if no second argument was given, _prepackCommand will be undefined.

cblappert · 2017-10-17T23:31:23Z

src/debugger/adapter/DebugAdapter.js

+    //set up message queue
+    this._messageQueue = new Queue();
+
+    this._prepackProcess = child_process.spawn("node", this._prepackCommand.split(" "));


spawn or spawnSync?

I think we need the asynchronous version here. The sychronous one will not return until the Prepack process has exited. But the adapter may need to run while Prepack is still running (e.g. passing state info to UI or passing new user commands to Prepack).

cblappert · 2017-10-17T23:44:11Z

src/debugger/mock-ui/DataHandler.js

+          for (let i = 0; i < lines.length; i++) {
+            let pair = lines[i].split(/: +/);
+            if (pair[0] === "Content-Length") {
+              this._contentLength = +pair[1];


unary + is confusing

cblappert · 2017-10-17T23:46:50Z

src/debugger/mock-ui/DataHandler.js

+      // if we know what length we are expecting
+      if (this._contentLength >= 0) {
+        // we have enough data to check for the expected message
+        if (this._rawData.length >= this._contentLength) {


Request Changes: Strictly speaking this just means that the buffer has allocated that much storage not necessarily that there are that many characters in the buffer. Please either make a comment or invariant around this if the behavior is intended.

Changed to compare against actual length in the buffer.

cblappert · 2017-10-18T00:00:51Z

src/debugger/mock-ui/UISession.js

+    let parts = input.split(" ");
+    let command = parts[0];
+
+    // for testing purposes, init and configDone are made into user commands


Maybe pose this comment as a TODO

For the CLI, we can keep these two commands as being from user input. This way we can control tests on them using the CLI. In an actual IDE, these will be done without user input.

cblappert · 2017-10-18T00:04:35Z

src/debugger/mock-ui/UISession.js

+
+    // for testing purposes, init and configDone are made into user commands
+    // they can be done from the adapter without user input
+    if (command === "init") {


Once we get more commands, a switch statement is probably better.

cblappert · 2017-10-18T00:06:59Z

src/debugger/mock-ui/UISession.js

+
+  // tell the adapter that configuration is done so it can expect other commands
+  _sendConfigDoneRequest(args: DebugProtocol.ConfigurationDoneArguments) {
+    this._sequenceNum++;


You could move the sequence num increment and stringify into the packageAndSend to make sure you can't forget to increment the sequence num.

Moved the increment into _packageAndSend. The stringify is harder to move because each request has a different type of arguments, so all the possible types would need to be specified in _packageAndSend.

jeffreytan81

Cool, looks good to me from high level, I will let @mostafaeweda to take a further look.

jeffreytan81 · 2017-10-18T16:43:03Z

src/debugger/mock-ui/UISession.js

+    this._invalidCount = 0;
+    this._dataHandler = new DataHandler();
+  }
+  // the parent (i.e. ui) process


It is not the parent process, it is mock UI process itself.

jeffreytan81 · 2017-10-18T16:47:06Z

src/debugger/adapter/DebugAdapter.js

+      process.exit(1);
+    }
+    //set up message queue
+    this._messageQueue = new Queue();


Is this holding all the pending commands from UI? Did not see any code using it. Will be used in later PR?

Yes, this will be used in the next PR when all 3 parts (DebugServer in Prepack, DebugAdapter, UI) are connected.

mostafaeweda · 2017-10-18T17:33:52Z

flow-typed/npm/vscode-debugprotocol_vx.x.x.js

+// Filename aliases
+declare module 'vscode-debugprotocol/lib/debugProtocol.js' {
+  declare module.exports: $Exports<'vscode-debugprotocol/lib/debugProtocol'>;
+}


This file doesn't seem very useful (it's mostly all any) & I can't find it in https://github.com/flowtype/flow-typed -- which should be the source of truth for any npm module types.

This file was autogenerated using the CLI from https://github.com/flowtype/flow-typed because it's not there already. It is needed to satisfy flow checks.

If the file doesn't add any value, you should just add it to the ignore section in .flowconfig.

mostafaeweda · 2017-10-18T17:36:41Z

src/debugger/adapter/DebugAdapter.js

+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+/* @flow */


nit: I see this project is setup with prettier in the devDependencies - but don't see the @format --> did you use prettier to format this file?

Our prettier seems to run off of directory structure instead of @Format annotations, everything matching src/**/*.js or scripts/**/*.js should get formatted.

mostafaeweda · 2017-10-18T17:38:07Z

src/debugger/adapter/DebugAdapter.js

+    this._prepackProcess = child_process.spawn("node", this._prepackCommand.split(" "));
+
+    process.on("exit", () => {
+      if (this._prepackProcess) this._prepackProcess.kill();


this._prepackProcess is never set to null --> so, no need to check for null -- OR (change the flow type to nullable and set to null here)

mostafaeweda · 2017-10-18T17:38:36Z

src/debugger/adapter/DebugAdapter.js

+
+    response.body = response.body || {};
+    response.body.supportsConfigurationDoneRequest = true;
+    //Respond back to the UI with the configurations. Will add more configurations gradually as needed.


nit space after //

mostafaeweda · 2017-10-18T17:46:34Z

src/debugger/mock-ui/DataHandler.js

+      break;
+    }
+  }
+}


this is duplicating Nuclide's V8Protocol.js & VsDebugSession.js (https://github.com/facebook/nuclide/blob/master/pkg/nuclide-debugger-common/lib/V8Protocol.js)
Also, there's the npm package: vscode-debugadapter-testsupport which'd help you setup and send individual events to the adapter in a promise-based fashion.

What would be the cleanest way to reuse those files and keep them in sync between the two repos?

We discussed offline that it'd be best to switch to using the vscode npm package.

I will use the npm package to set up automated testing.

mostafaeweda · 2017-10-18T17:47:17Z

src/debugger/mock-ui/UISession.js

+    this._adapterProcess.kill();
+    this._proc.exit(0);
+  }
+}


Again, this is all duplicated work

mostafaeweda · 2017-10-18T17:48:38Z

src/debugger/mock-ui/debugger-cli.js

+
+/* @flow */
+
+import { UISession } from "./UISession.js";


This should better wrap Nuclide's VsDebugSession or use VSCode's utils instead.

Summary: - Mock UI as a CLI to communicate with the adapter - Starter code for the debugger adapter with an initialize request Test: - Ran the mock-ui, can send the initialize request and receive a response on the CLI

- fix check for buffer size - change if to switch for commands

facebook-github-bot

@JWZ2018 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Release note: none Issue: #907 - Mock UI as a CLI to communicate with the adapter - Starter code for the debugger adapter with an initialize request Closes #1088 Differential Revision: D6108024 Pulled By: JWZ2018 fbshipit-source-id: 506ec039dcb1ff540872cb2e1a87a165cfd22e4d

jwli229 requested review from cblappert, hermanventer, NTillmann and jeffreytan81 October 16, 2017 19:02

facebook-github-bot added the CLA Signed label Oct 16, 2017

hermanventer reviewed Oct 16, 2017

View reviewed changes

cblappert suggested changes Oct 18, 2017

View reviewed changes

jeffreytan81 approved these changes Oct 18, 2017

View reviewed changes

mostafaeweda suggested changes Oct 18, 2017

View reviewed changes

jwli229 added 9 commits October 19, 2017 17:13

Framework for debug adapter and mock ui

dfbde2a

Summary: - Mock UI as a CLI to communicate with the adapter - Starter code for the debugger adapter with an initialize request Test: - Ran the mock-ui, can send the initialize request and receive a response on the CLI

Add in npm vscode debug modules

b28d3f1

Move data handler to a separate class

d9b39da

Move data handler to a separate class

39b0c6e

Move data handler to a separate class

2adb84c

Fix Prepack start up in adapter

8117722

Fix checks and use switch

e20c9c4

- fix check for buffer size - change if to switch for commands

Remove unneeded checks

b8a8390

Remove unneeded flow-typed stubs

684c1bb

jwli229 force-pushed the debugadapter branch from 8e397ee to 684c1bb Compare October 20, 2017 00:42

cblappert approved these changes Oct 20, 2017

View reviewed changes

facebook-github-bot reviewed Oct 20, 2017

View reviewed changes

facebook-github-bot closed this in f6279bd Oct 20, 2017

jwli229 deleted the debugadapter branch October 20, 2017 23:10

Framework for debug adapter and mock ui #1088

Framework for debug adapter and mock ui #1088

Conversation

jwli229 commented Oct 16, 2017 • edited Loading

hermanventer commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwli229 Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwli229 commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreytan81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

jwli229 commented Oct 16, 2017 •

edited

Loading

jwli229 Oct 17, 2017 •

edited

Loading