svaante / dape

Debug Adapter Protocol for Emacs
GNU General Public License v3.0
455 stars 25 forks source link

Improve documentation for setting up nodejs debugger processes #44

Closed timcharper closed 7 months ago

timcharper commented 8 months ago

A few things weren't clear to me:

After an hour or two of debugging and hacking, I arrived at the following. I usually learn best from examples, maybe we could share this as one of the examples?

(defun dape-jest/find-file-buffer-default ()
  "Read filename at project root, defaulting to current buffer. Return vector of jest args to run said file"
  (let ((file (dape-find-file (buffer-file-name))))
    (if file
        `["--runInBand" "--no-coverage" ,file]
      (user-error "No file found"))))

(defun dape-jest/ensure (config)
  "Ensure node is available, jest is installed, that the dapDebugServer is installed"

  (dape-ensure-command config)
  (let ((js-debug-file (file-name-concat
                        (dape--config-eval-value (plist-get config 'command-cwd))
                        (dape--config-eval-value (car (plist-get config 'command-args)))))
        (node-jest-file (file-name-concat
                         (dape--config-eval-value (plist-get config :cwd))
                         (dape--config-eval-value (plist-get config :program)))))
    (unless (file-exists-p js-debug-file)
      (user-error "Debug server file %S does not exist" js-debug-file))
    (unless (file-exists-p node-jest-file)
      (user-error "Jest executable not found at %S" node-jest-file))))

;; setup jest configuration
(setq dape-configs 
      `((jest
         modes
         (js-mode js-ts-mode typescript-mode)
         ensure dape-jest/ensure
         command "node"
         command-cwd ,(file-name-concat dape-adapter-dir
                                        "js-debug")
         command-args (,(file-name-concat "src" "dapDebugServer.js")
                       :autoport)
         port :autoport
         fn dape-config-autoport
         :type "pwa-node"
         :cwd dape-cwd-fn
         :program "node_modules/.bin/jest"
         :args dape-jest/find-file-buffer-default
         :outputCapture "console"
         :sourceMapRenames t
         :pauseForSourceMap nil
         :autoAttachChildProcesses t
         :console "internalConsole"
         :killBehavior "forceful")))

Explanation:

The resulting launch command will look something like this:

{
  "type": "request",
  "command": "launch",
  "arguments": {
    "type": "pwa-node",
    "cwd": "/path/to/project",
    "program": "node_modules/.bin/jest",
    "args": ["--runInBand", "--no-coverage", "path/to/my-module-test.ts"],
    "outputCapture": "console",
    "sourceMapRenames": true,
    "pauseForSourceMap": null,
    "autoAttachChildProcesses": true,
    "console": "internalConsole",
    "killBehavior": "forceful"
  }
}

(notice that only keyword args, or those starting with a :, are passed as arguments to the launch command)

...

I want to add, I can open up a PR for this. I just wanted to get a discussion going on it first and your thoughts.

timcharper commented 8 months ago

I'm not sure I love the adapter being flattened in with the program to be run... this means I have to repeat the adapter configuration for different run configurations (debug all jest tests, debug specific jest file, debug test closest to cursor...). Having a separate list of adapters and then referencing them by name is what nvim-dap does and I think perhaps other implementations?

joaotavora commented 8 months ago

I'm not sure I love the adapter being flattened in with the program to be run..

I don't want to pile on this gratuitously, but I made the same point some time ago I think :-) This is a pain point of DAP in general, and I also think it should cleanly separate. My "zapp" experiments do, and at least try to make it clear what gets sent where. I think that part is OK (most the rest is 💩 for now).

https://github.com/joaotavora/zapp/blob/master/zapp.el

if you're interested, and here's what the minibuffer interface looks like (it looks more convoluted than it actually is :-), the bottom part can be turned off and hints at what will really be run, the top part is just a string of usual persistent history)

image

The zapp-method: is the DAP command, can be :attach or :launch. The zapp-class: designates an Elisp class where you can stash even more server eccentricities. It's guessed from the name of the binary, usually correctly, and can also be overriden.

Anyway, just an idea.

timcharper commented 8 months ago

That looks pretty cool.

One thing that'll I'll say about the existing shape setup is that the history works really well. A plist for launching your job.

I think if we just separate out adapters and run configurations we're golden. I don't think much should change UX wise, just config wise.

joaotavora commented 8 months ago

One thing that'll I'll say about the existing shape setup is that the history works really well. A plist for launching your job.

Yeah, I noticed that very good idea too, so I copied into Zapp, with changes. Dape's code to load/save history is complicated the the UX not obvious, whereas Zapp tries to strike a balance between normal shell invocations and optional Lisp things, so less customization of variables in .el files is needed when trying our new debuggers or project configurations. For example, the much simpler and familiar incantation python -m debugpy.adapter also works (or at least should 😆 ) and it's just a shell command.

I think if we just separate out adapters and run configurations we're golden. I don't think much should change UX wise, just config wise.

Config is UX too :-) Less of it is better UX, IMO. But yes, Dape's UX is generally well though out with ergonomic, integration and minimal reinvention in mind. The suggestion of repeat-mode is very interesting (haven't got to that part yet).

svaante commented 8 months ago

I don't know where to strike the balance between batteries included and customization. It feels like a jest specific configuration might be more of a wiki thing but I am not sure.

I'm not sure I love the adapter being flattened in with the program to be run... this means I have to repeat the adapter configuration for different run configurations (debug all jest tests, debug specific jest file, debug test closest to cursor...). Having a separate list of adapters and then referencing them by name is what nvim-dap does and I think perhaps other implementations?

I agree, first intention of dape was to keep the configuration part on the user as https://github.com/mfussenegger/nvim-dap does it, but I caved in. Then I added the batteries without much thought and now it's the part of dape that I like the least. I am extremely open to suggestions!

I have had a couple of unformed thoughts about dape-config.

But I am open for a redesign as well. hint hint @joaotavora

timcharper commented 8 months ago

Yeah, sounds like there's general agreement separating out adapter from launch config.

As for jest - yeah, it's super specific. I think a wiki would do nicely. I wonder what level the core documentation could explain the process of setting up a new process, however. Maybe the wiki is fine. It just feels like there needs to be a bit more explanation.

svaante commented 8 months ago

Some changes has been suggested in #52 to get them showing up nicely in the minibuffer hints dape-jest/find-file-buffer-default needs the property `dape--minibuffer-hint'. If you have time I greatly appreciate feedback as well. (@joaotavora @timcharper)

timcharper commented 7 months ago

I just upgraded to dape 0.5.0 and the feedback hints are really cool!

I've added the 'dape--minibuffer-hint property, but I'm not exactly sure what it does / changes.

I went ahead and put the example on the wiki:

https://github.com/svaante/dape/wiki#debug-jest-unit-test

timcharper commented 7 months ago

Closing as the resolution was to update the wiki

svaante commented 7 months ago

Would you be fine with crediting you with adding the section?

svaante commented 7 months ago

When an function symbol has the dape--minibuffer-hint property the result of the function is displayed as an hint when reading configuration.

That reminds me that I should probably document it somewhere ;)

timcharper commented 7 months ago

Yeah, totally, I can add a link to myself for it so people know who to bug

timcharper commented 7 months ago

(done)