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

Adds support for the node adapter #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

eastwood
Copy link

@eastwood eastwood commented Nov 4, 2022

As of July 2022, the pwa-node adapter became the default node adapter and is now available under the node type by default.

In this PR, I've updated the config filter to support the launch configuration containing the node type. I've left the existing pwa-node type for backwards compatibility until such time that the pwa-node adapter will become officially deprecated.

Essentially, this is a name change - so I've copied the tests across to make sure that we can catch any funny business if the two adapter diverge.

I'm very new to lua, so have used your workflow to run test cases, and change requests are encouraged and welcome :)

@sspatari
Copy link

sspatari commented Nov 8, 2022

@mxsdev can you look at this?

@mxsdev
Copy link
Owner

mxsdev commented Nov 13, 2022

Hi! Thank you so much for the pr and for being open to change requests :)

If I understand correctly, this is in reference to microsoft/vscode-js-debug#1305? If so, I believe those changes apply only at the external configuration level (think VSCode's launch.json).

Correct me if I am wrong, but I think that your tests still use the old launch_config - so it appears exactly identical to the old tests. Trying to run with the type as node in my tests results in an error, which is caused by this line of the debugger.

Basically, there is an extra step between a client like VSCode and the internal debugger, which replaces node with pwa-node (see here). I'm pretty sure this layer of indirection happens client-side, not server-side.

Since there is the possibility of pwa-* being removed internally, here is my proposal going forward for this project:

  • Allow node, chrome, msedge, and extensionHost as options, and alias them to the pwa-* equivalents internally
  • Mark the pwa-* dap types as "deprecated," and show a warning to the user on dap initialize that these debug types are officially deprecated and may eventually stop working
  • Give a config option to hide this warning for users who know what they're doing

Then if down the road pwa_node gets totally wiped internally and it is replaced by node, we can just remove the alias and forbid pwa_node for all versions of vscode-js-debug not sufficiently new.

Let me know how you feel about this approach. You say you're not super familiar with lua so I'm happy to implement it myself, just let me know 👍

@mxsdev mxsdev mentioned this pull request Mar 3, 2023
@dieguit
Copy link

dieguit commented Apr 25, 2023

Hello! Thanks @mxsdev for looking into this.
I have an issue with this node vs pwa-node naming, so I'm wondering how this PR will proceed.
Basically, I am using a launch.json file that has stuff like

    {
      "name": "some-service",
      "type": "node",
      "request": "attach",
      "port": 9000
    },

For this to work, after adding the pwa-node adapter with this plugin I need to:

  1. Add another entry (or change the existing one so it's
    {
      "name": "some-service",
      "type": "pwa-node",
      "request": "attach",
      "port": 9000
    },
  1. Update the launchjs to be:
require('dap.ext.vscode').load_launchjs(nil, {
  ['pwa-node'] = {
    'javascript',
    'typescript',
  },
  ['node'] = {
    'javascript',
    'typescript',
  },
})

I'm not sure if I'm missing something or this could be done in a better way, but i'd be nice if we were able tu use just node in the launch.json entry

@80avin
Copy link

80avin commented Apr 20, 2024

@dieguit You can do it like this

local ok, filetypes = pcall(require, 'mason-nvim-dap.mappings.filetypes')
require("dap.ext.vscode").load_launchjs(nil, ok and filetypes or nil)

@80avin
Copy link

80avin commented Apr 20, 2024

@eastwood Is there any progress on this ?

I don't want to do duplicate efforts but if this PR is abandoned, I'll prefer to work on another PR

@80avin
Copy link

80avin commented Apr 20, 2024

Hi @mxsdev , I'm thinking of completing this PR and need one clarification on the suggestions you've given.

Allow node, chrome, msedge, and extensionHost as options, and alias them to the pwa-* equivalents internally

Is it correct or otherwise ? I think logical is to alias the pwa-* equivalents to node, chrome, msedge and extensionHost equivalents. i.e. If user provides "pwa-node", it should be mapped to "node" and used so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants