fix: updated debugAdapterPath to use pwa-node as node debug type #147
Conversation
| @@ -216,7 +216,7 @@ | |||
| "description": "Debugger configuration for Alexa skills (Node.js)", | |||
| "body": { | |||
| "name": "Debug Alexa Skill (Node.js)", | |||
| "type": "node", | |||
| "type": "pwa-node", | |||
There was a problem hiding this comment.
why it's pwa-node ? I didn't see it mentioned in the changelog. Does this type work for all Nodejs developers ?
There was a problem hiding this comment.
So you tested the local debug feature in both 1.60 and before 1.60 ? I wonder if we change the config, will people who use old version vscode can still use local debug ? If not probably we need to provide a separate config for after 1.60
Yes tested with >v.1.60 with the changes and also launched a local debugging session on test skill to verify the fix.
Why do we need to test before v.1.60 ? Shouldn't we ask developers to update their IDE version its a minor release bump ??
There was a problem hiding this comment.
why it's pwa-node ? I didn't see it mentioned in the changelog. Does this type work for all Nodejs developers ?
Testing/debugging through the codebase here , type is returned as pwa-node instead of node on latest release.
There was a problem hiding this comment.
Yeah, just wonder if this value will change on different test environment, like different node version, different vscode version
There was a problem hiding this comment.
Not sure what's the best practice for us if the behavior is different for developers use old version IDE.
If this change will break exsiting developers on old version ide, then it will be a breaking change right ? Like develoepr may have some reasons to use old version IDE, and I think we'd better not break them ? Just my personal thoughts, we can have some discuss within team and see what others think if it's the case
There was a problem hiding this comment.
I tested node v12 and v14 and also previous builds on VsCode IDE (v.1.58 and v1.59) and the change pwa-node works since it was available as preview mode. So we should be good to update our Configs as well. :)
Thanks for pointing it out @ShenChen-Amazon !
| if (!aplPreviewPanel || !aplPreviewPanel.visible) { | ||
| throw loggableAskError(ERROR_MESSAGES.CHANGE_VIEWPORT_PROFILE_NO_APL_PREVIEW, undefined, true); | ||
| if (aplPreviewPanel === undefined || !aplPreviewPanel.visible) { | ||
| throw logAskError(ERROR_MESSAGES.CHANGE_VIEWPORT_PROFILE_NO_APL_PREVIEW, undefined, true); |
There was a problem hiding this comment.
why change loggableAskError to logAskError in this PR ?
There was a problem hiding this comment.
It was causing spellCheck errors, so refactored(renamed) it to logAskError any issue on changing ?
There was a problem hiding this comment.
I am not clear the differences between these two. I think would be great if you could dig into them and find the differences ?
|
Sorry - hit wrong button and lost Shen's approval. But looks good to me as well. |
Description
On recent VsCode IDE update >v.1.60.* debugging configuration on node has changed
See Changelog and the node debugging configutations are of
type: pwa-node.Updated our extension codebase to reflect this change and added test cases.
Testing
Screenshots (if appropriate)
Types of changes
Checklist
License