-
Notifications
You must be signed in to change notification settings - Fork 280
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
Reintroduce js-debug auto attach #703
Comments
Another option brought up in standup: attach based on the script being run. E.g. don't attach if running "npm" or "webpack", but do attach if running "node" directly. |
In the next nightly (which will work on the following Insiders, opt-in), I have done the following:
Following up on ENOENT, looking back at the issues. The vast majority were either:
There was only one case where the workspace storage might have gone missing, and that could have been a version detection issue. Thus I am not sure if that is a problem. Looking at the terminal variables code in VS Code, it seems like they're kept in the workspace storage as well. Once Insiders is published tomorrow I will go through a bunch of test VMs and setups to make sure everything is solid. |
This will, by default, try to only attach when running node scripts directly, and avoid attaching to tooling installed globally or in dependencies. See #703
Further item from standup today: allowlist common test runners to enable auto attach for them. Also, ts-node. |
Also, cc @weinand in case you're interested in following this 🙂 |
@connor thanks for cc-ing me. Yes, I vote for "explicit auto attach" because that's the way it worked before, and existing users understand how that works. In addition it is less aggressive in debugging everything (like for example the I wasn't aware of the bootloader's fragility with respect to spaces in paths and node.js versions. Showing a notification in these situations is definitively better than disabling auto-attach silently. "attach based on the script being run" sounds "too magic" for my taste and is not really transparent. Who owns the list of magic commands for which auto-attach is suppressed? Are users able to add new commands to that list? I suggest to keep the bootloader in a location that does not have a path that contains the user name. |
I was originally in favor of that as well, but Kai's convincing point was that only a small percentage of VS Code users use debugging at all. While defaulting to "explicit" attach mode as we do today makes these users happy, it does not help the vast majority of users who don't know about/use debugging. But if I run Meanwhile, auto attaching to build scripts and such is very annoying, so attaching to everything is not a good solution. Thus I landed on the smart attach mode.
Currently smart attach will only attach when running scripts not installed globally and not in node_modules -- so it's suppress by default. For example, this prevents attaching to tools like webpack, tsc, or npm. We would have an allowlist of modules where we want to lift this surpression, such as mocha, jest, or ts-node. I expect to receive a steady trickle of PRs adding more tools to the list over time. Making it configurable in a setting is a good idea -- we could add a denylist as part of that as well.
That's what we did originally. The problematic thing is that the OS could clear the temp directory while the environment variables were still present in the VS Code terminal. This would cause all Node commands run in the integrated terminal to fail when they could not find the module (e.g. microsoft/vscode#102557) Because this breaks in such a severe way, in re-enabling js-debug auto attach I wanted to be very conservative with the way we enable it. |
We swapped auto attach to js-debug in the June release with destructive results, and so reverted back to node-debug's auto attach in a recovery.
--inspect
flags manually.--inspect
is given.We'd like to re-enable js-debug auto attach this coming iteration:
The text was updated successfully, but these errors were encountered: