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

Use Inspector instead of Legacy Debugger #285

Merged
merged 7 commits into from
Feb 27, 2018
Merged

Use Inspector instead of Legacy Debugger #285

merged 7 commits into from
Feb 27, 2018

Conversation

charsleysa
Copy link
Contributor

Changes NodeJS 6.10 runtime debugging to use Inspector instead of Legacy Debugging.

A minor breaking change as debug tools need to have their config updated to use the Inspector protocol.

e.g. for VSCode inside launch.json you change "protocol": "legacy", to "protocol": "inspector",

- Change Node 6.10 to use inspector debugging instead of legacy debugging
- Correct commands for node 6.10
- Don't overwrite headers
@uipoet
Copy link

uipoet commented Feb 16, 2018

The legacy debugger in VSCode is useless as breakpoints are not working. Please merge this pull request as soon as possible.

@PaulMaddox
Copy link
Contributor

Could you update the README to reflect this required change too? Once that's added i'm happy to merge this in and get it included in the next release. Thanks!

@singulli1
Copy link

FYI - this is an important fix for my team as well - we use the debugger extensively. It is currently broken. Any movement on this would be greatly appreciated :). Thanks so much.

- Updated VSCode launch config
- Added note about detaching debugger
@charsleysa
Copy link
Contributor Author

@PaulMaddox updated README

@PaulMaddox
Copy link
Contributor

Thanks @charsleysa!

@PaulMaddox PaulMaddox merged commit 2c3cb7c into aws:develop Feb 27, 2018
@PaulMaddox
Copy link
Contributor

There seems to be an issue with this PR - when i'm testing it, turning on debugging doesn't halt execution - it should automatically halt on the first line of code so you can attach the debugger.

I'll do some more testing later in the week to try and see what's up.

@singulli1
Copy link

Hello, were you able to resolve the issue with the PR? is this item fixed in the develop branch?
Thanks again for all your work.

@bradydowling
Copy link
Contributor

I'm also having an issue with this PR/commit. It looks like --inspect does not report end of execution (in Node 6.10) so the debugger never knows when to disconnect. This PR was made and merged for Node 7+ but never backported to Node 6.10.

Because of this, IDEs can't know when the debugger should detach. Is the note in the README suggesting that the user needs to manually detach the debugger upon program completion? This feels hackish.

@PaulMaddox is there a certain fix you implemented to get it to halt execution? Or was there a change you made in the IDE plugin?

@bradydowling
Copy link
Contributor

I'm also curious, what caused the legacy debugger to break in VSCode? Was it working at some point and then it was broken by a change in SAM Local?

@singulli1
Copy link

singulli1 commented Mar 28, 2018 via email

@bradydowling
Copy link
Contributor

Ah that makes sense. Perhaps there should be an environment variable that allows you to override the run command altogether.

I added an environment variable to allow for additional debugger args but replacing the command altogether may be preferable.

This could potentially future proof SAM Local from breaking changes or needs like this from other services or IDEs. If a certain IDE breaks or requires certain arguments, you can just pass that environment variable and override the whole run command.

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