wp-scripts test-unit-js node debugging #21631
Conversation
ajlende
requested review from
ajitbohra,
chrisvanpatten,
gziolo,
nerrad,
ntwb and
youknowriad
as code owners
gziolo
added
[Tool] WP Scripts
|
|
||
| A breakpoint will be set at the first line of the script (this is done to give you time to open the developer tools and to prevent Jest from executing before you have time to do so). Click the resume button in the upper right panel of the dev tools to continue execution. When Jest executes the test that contains the debugger statement, execution will pause and you can examine the current scope and call stack. | ||
|
|
||
| ##### Debugging in Visual Studio Code |
There was a problem hiding this comment.
The same question applies, what about other IDEs?
There was a problem hiding this comment.
Node's inspector clients section lists Visual Studio, JetBrains IDEs, Gitpod, and Eclipse. I expect Gitpod and Visual Studio to be similar to Code since they're all quite similar IDEs, but I haven't used JetBrains or Eclipse for web development, so I can't comment on how different they might be to set up for debugging.
It's probably not reasonable to add all the options, but I figured it would be good to include at least a couple specific graphical options as examples in the README since documentation I've seen elsewhere isn't very great. And debugging these scripts is going to be slightly different than the normal use-case because our script spawns the node process rather than the user running node directly with the options.
There was a problem hiding this comment.
I have a similar feeling here. The section could target IDEs in general and could include the details you shared in the comment and we would just have this example for VSC included as is. The truth is that VSC and Chrome are the most popular anyway 😃
packages/scripts/bin/wp-scripts.js
Outdated
|
|
||
| const [ scriptName, ...nodesArgs ] = getArgsFromCLI(); | ||
| const { scriptName, scriptArgs, nodeArgs } = getNodeArgsFromCLI( [ | ||
| '--inspect-brk', |
There was a problem hiding this comment.
I did some research and it looks like node take can a large number of CLI options:
https://nodejs.org/api/cli.html
Do you know whether other options would be useful here?
What other approaches did you consider?
One thing I discovered is -- option:
Indicate the end of node options. Pass the rest of the arguments to the script. If no script filename or eval/print script is supplied prior to this, then the next argument will be used as a script filename.
It looks like a perfect fit here but I don't know how it would play out in this specific case.
We already use minimist package for parsing args and it has support for this option:
https://www.npmjs.com/package/minimist#var-argv--parseargsargs-opts
There was a problem hiding this comment.
Do you know whether other options would be useful here?
I suppose adding additional options could be useful for some people in other special cases for things like debugging the Webpack build, but nobody has asked for that yet, and it's easy enough to add things on a case-by-case basis.
I chose to do the whitelist approach for node options because it was the most straightforward way to implement this while maintaining backwards compatibility and keeping the niceness on not having to maintain a list of available scripts in the code.
The only options that I've personally used are --inspect, --eval (instead of awk for JSON files), and a few of the --experimental options (also probably not useful in this context). Maybe I'm missing out on some really useful options, haha. --heapsnapshot-signal and --prof look pretty cool, but they're the sort of things that I would typically debug in the browser rather than while running unit tests.
What other approaches did you consider?
The first thing I looked at was using NODE_OPTIONS environment variable, but that also applies the node args to wp-scripts.js too, and the debugger tries to start twice but can't because they're running on the same port.
Similarly, a WP_SCRIPTS_NODE_OPTIONS environment variable could be used—we'd just have to add a dependency to parse those args before passing them through. This also has the potential to be used alongside passing the args before the script if people would find that useful.
I hadn't considered it before, but you could add a custom delimiter similar to -- (see next question for why it can't be --) to separate the node scripts. --wp-script would make sense when you read through the whole command, wp-scripts --inspect-brk --wp-script test-unit-js --runInBand --no-cache. That would make it easier to allow all the node options and still not have to maintain a list of scripts in the code.
Still, considering how similar wp-scripts is to react-scripts, I think it would be best to follow the same pattern they use. Their way of passing args seems fine from the user's perspective, and if some people are already familiar with how it works there, it doesn't make sense to me to do something different that might cause confusion unless the new way was going to fulfill a unique use-case for us.
The
--option looks like a perfect fit here but I don't know how it would play out in this specific case.
The -- option is already being used via minimist to pass additional options to some scripts (files/directories to format in format-js and webpack args in start), so it was a good idea, but I wouldn't want to change how those two scripts work.
There was a problem hiding this comment.
Still, considering how similar
wp-scriptsis toreact-scripts, I think it would be best to follow the same pattern they use. Their way of passing args seems fine from the user's perspective, and if some people are already familiar with how it works there, it doesn't make sense to me to do something different that might cause confusion unless the new way was going to fulfill a unique use-case for us.
Thanks for reminder about react-scripts, it’s the first place I should check. This is how they have implemented it:
They have a very simple approach that addresses my main concern which is the list of node args that needs to be maintained on our side. I wouldn’t mind if we replicate this implementation where they find the index that matches one of the script names and treat everything before as node args. They have only 4 script names there so they hardcode them, but we can get all names from the scripts subfolder based on the files located there.
There was a problem hiding this comment.
@gziolo Thanks for taking the time to research some things and review! You had a lot of good questions 🙂
I added some comments on why I think the arguments should be passed the way they are, but I can go ahead and change it if you read through and still like one of the other options better
|
|
||
| A breakpoint will be set at the first line of the script (this is done to give you time to open the developer tools and to prevent Jest from executing before you have time to do so). Click the resume button in the upper right panel of the dev tools to continue execution. When Jest executes the test that contains the debugger statement, execution will pause and you can examine the current scope and call stack. | ||
|
|
||
| ##### Debugging in Visual Studio Code |
There was a problem hiding this comment.
Node's inspector clients section lists Visual Studio, JetBrains IDEs, Gitpod, and Eclipse. I expect Gitpod and Visual Studio to be similar to Code since they're all quite similar IDEs, but I haven't used JetBrains or Eclipse for web development, so I can't comment on how different they might be to set up for debugging.
It's probably not reasonable to add all the options, but I figured it would be good to include at least a couple specific graphical options as examples in the README since documentation I've seen elsewhere isn't very great. And debugging these scripts is going to be slightly different than the normal use-case because our script spawns the node process rather than the user running node directly with the options.
packages/scripts/bin/wp-scripts.js
Outdated
|
|
||
| const [ scriptName, ...nodesArgs ] = getArgsFromCLI(); | ||
| const { scriptName, scriptArgs, nodeArgs } = getNodeArgsFromCLI( [ | ||
| '--inspect-brk', |
There was a problem hiding this comment.
Do you know whether other options would be useful here?
I suppose adding additional options could be useful for some people in other special cases for things like debugging the Webpack build, but nobody has asked for that yet, and it's easy enough to add things on a case-by-case basis.
I chose to do the whitelist approach for node options because it was the most straightforward way to implement this while maintaining backwards compatibility and keeping the niceness on not having to maintain a list of available scripts in the code.
The only options that I've personally used are --inspect, --eval (instead of awk for JSON files), and a few of the --experimental options (also probably not useful in this context). Maybe I'm missing out on some really useful options, haha. --heapsnapshot-signal and --prof look pretty cool, but they're the sort of things that I would typically debug in the browser rather than while running unit tests.
What other approaches did you consider?
The first thing I looked at was using NODE_OPTIONS environment variable, but that also applies the node args to wp-scripts.js too, and the debugger tries to start twice but can't because they're running on the same port.
Similarly, a WP_SCRIPTS_NODE_OPTIONS environment variable could be used—we'd just have to add a dependency to parse those args before passing them through. This also has the potential to be used alongside passing the args before the script if people would find that useful.
I hadn't considered it before, but you could add a custom delimiter similar to -- (see next question for why it can't be --) to separate the node scripts. --wp-script would make sense when you read through the whole command, wp-scripts --inspect-brk --wp-script test-unit-js --runInBand --no-cache. That would make it easier to allow all the node options and still not have to maintain a list of scripts in the code.
Still, considering how similar wp-scripts is to react-scripts, I think it would be best to follow the same pattern they use. Their way of passing args seems fine from the user's perspective, and if some people are already familiar with how it works there, it doesn't make sense to me to do something different that might cause confusion unless the new way was going to fulfill a unique use-case for us.
The
--option looks like a perfect fit here but I don't know how it would play out in this specific case.
The -- option is already being used via minimist to pass additional options to some scripts (files/directories to format in format-js and webpack args in start), so it was a good idea, but I wouldn't want to change how those two scripts work.
|
Thanks for all your answers. It helped me better understand the decision process and I learned a lot about Node a ton 😅 I don’t think there is anything wrong with the implementation part of PR as is and I would be fine with landing it soon. I still need to be tested though and I plan to take care of it later this week. |
|
@youknowriad – can you promote Alex to the member role, it's well deserved :) |
|
I opened a follow-up with the changelog entry added for the package to make it easier to discover: #21861. |
|
Thanks for doing this! |
Description
Closes #19920
--inspectflags to node when running scripts in a method similar to how create-react-app allows them to be passed.test-unit:debugscript with the flag in package.json.How has this been tested?
spawnScript.Types of changes
New feature, non-breaking change which adds functionality.
Checklist: