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

Add option to pass --inspect flag to enable server-side debugging #3294

Merged
merged 9 commits into from
Nov 20, 2017

Conversation

auchenberg
Copy link
Contributor

Hello from the VS @code team,

I'm currently looking into enabling next.js to debugged from VS Code in microsoft/vscode-recipes#31, and after some research I didn't seem possible to start next with Node's --inspect flag that starts the Node.js debugging server. This PR modifies the CLI to enable the ``--inspect` flag to be passed to the spawned child process, and enables debugging of the server-side rendering in VS Code (and Chrome DevTools).

Steps to validate in Chrome DevTools:

  1. Run next --inspect or next dev --inspect
  2. Wait for server to start
  3. Open Chrome DevTools, and you should notice the Node icon has turned green. Click it and you can set a breakpoint in the server-side code such as pages and break on getInitialProps, which is used for SSR.

Add option to pass --inspect flag to enable server-side debugging

@roblourens
Copy link

I think it should also handle --inspect-brk, and --inspect=1234

bin/next Outdated

const startProcess = () => {
const proc = spawn(bin, args, { stdio: 'inherit', customFds: [0, 1, 2] })
const proc = spawn(node, args, { stdio: 'inherit', customFds: [0, 1, 2] })
Copy link
Member

@timneutkens timneutkens Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be bin? = const bin = join(__dirname, 'next-' + cmd) 🤔

Copy link
Contributor Author

@auchenberg auchenberg Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I've changed the scripts to be regular JavaScript files, that doesn't contain #!/usr/bin/env node as we need to launch Node with the --inspect flag and having the files as a shell script doesn't allow that.

So the change here is the that we spawn a child process like node --inspect <path to next-dev>

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you very much @auchenberg ❤️ Just had one question.

@timneutkens
Copy link
Member

@auchenberg can you check the CI results 👍

bin/next-build Outdated
@@ -1,4 +1,3 @@
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need to change this? While I have never seen anyone running next-build or next-dev commands (instead of next build / next dev, where next command gets executed), this seems to be a breaking change to me, and given the files locate in the bin folder, it makes sense to be able to execute them (so shebang is necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change their, but I haven't found a way to pass the --inspect flag to #!/usr/bin/env node otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@auchenberg having this shebang doesn't require you use them. Just leave shebang even if you are going to run them from next using explicit node --inspect next-build.

@auchenberg
Copy link
Contributor Author

@timneutkens @frol The PR has been simplified, and CI is passing now. Windows CI seems to be a bit flaky.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@timneutkens timneutkens merged commit 20af8cd into vercel:canary Nov 20, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Nov 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants