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

fix(debugger): accept arbitrary ip and port as NODE_OPTIONS #53683

Closed
wants to merge 2 commits into from

Conversation

HelmerBarcos
Copy link

@HelmerBarcos HelmerBarcos commented Aug 7, 2023

What?

This enables debugging sessions from a remote host. This is helpful while debugging a Next.js application that runs inside a Docker container with following possibilities inside the package.json:

    "dev": "NODE_OPTIONS='--inspect=localhost:9250' next dev",
    "dev": "NODE_OPTIONS='--inspect=co.localhost:9250' next dev",
    "dev": "NODE_OPTIONS='--inspect=barcos.co:9250' next dev",
    "dev": "NODE_OPTIONS='--inspect=barcos.co.co.hzoe.com:9250' next dev",
    "dev": "NODE_OPTIONS='--inspect=0.0.0.0:9250' next dev",
    "dev": "NODE_OPTIONS='--inspect=9250' next dev",
    "dev": "NODE_OPTIONS='--inspect' next dev",
    "dev": "NODE_OPTIONS='--inspect=127.0.0.1:9250' next dev",

Bug

This fixes:

Other "fixed" bugs

Why?

It was an attempt to fix the bug, but it didn't work for all the cases.

Keep in mind that the default host will be 127.0.0.1, and this will be blocked for external hosts trying to attach to the debugger. The NODE_OPTIONS should be set to NODE_OPTIONS='--inspect=0.0.0.0:SOME_PORT'.
https://nodejs.org/en/docs/guides/debugging-getting-started#security-implications

@vleeuwenmenno
Copy link

Is there a version we can use that does not have the bug that is fixed in this PR?
This PR needs priority ...

@timmywil
Copy link
Contributor

Agreed, would be great to prioritize this issue and get a fix in 13.4.20.

SatsAllDay added a commit to SatsAllDay/stacker.news that referenced this pull request Aug 28, 2023
Update ports exposed, and run the next server with inspect flag

Currently does not work due to bug in next.js
See related PR:
vercel/next.js#53683
Comment on lines 444 to 590
const testAllIPsAndPorts = async (someIp) => {
const testIps = [
'0.0.0.0',
'127.0.0.1',
'localhost',
'192.168.18.90',
'barcos.co',
'app.barcos.co',
]
for (const someIp of testIps) {
const port = await findPort()
test(`NODE_OPTIONS='--inspect=${someIp}:${port}'`, async () => {
let output = ''
const app = await runNextCommandDev(
[dirBasic, '--port', port],
undefined,
{
onStdout(msg) {
output += stripAnsi(msg)
},
env: { NODE_OPTIONS: `--inspect=${someIp}:${port}` },
}
)
try {
await check(() => output, new RegExp(`on ${someIp}:${port}`))
await check(() => output, new RegExp(`http://${someIp}:${port}`))
} finally {
await killApp(app)
}
})
}
}

testAllIPsAndPorts()

Copy link
Member

Choose a reason for hiding this comment

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

Looks good! But test case names should be determanistic, not dynamically generated.

Could you update this such that the for loop is inside the test case? So you would have something like:

test("NODE_OPTIONS='--inspect=<host>:<port>'", async () => {
  const hosts = [
    '0.0.0.0',
    '127.0.0.1',
    'localhost',
    '192.168.18.90',
    'barcos.co',
    'app.barcos.co',
    null,
  ]
  for (const host of hosts) {
    const port = await findPort()
    // ...
  }
})

Copy link
Author

@HelmerBarcos HelmerBarcos Oct 23, 2023

Choose a reason for hiding this comment

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

Thanks. I made the changes just a couple of minutes ago; I had a lot of work to do

Copy link
Author

Choose a reason for hiding this comment

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

@wyattjoh, do I need to do anything else? This MR could be beneficial for many developers who prefer not to debug using console logs.

@mkoreo
Copy link

mkoreo commented Oct 17, 2023

Is there a version we can use that does not have the bug that is fixed in this PR? This PR needs priority ...

Indeed, this issue has been going on for 7-8 months now in which it is completely impossible to debug next.js in a docker container. Is it possible to give the review of these 10 lines of code priority?

@timneutkens
Copy link
Member

The PR was already reviewed but the comments were not addressed yet, so this can't be landed as-is. Feel free to open a new PR based on this one to fix that, happy to land it when the comments are addressed.

@HelmerBarcos
Copy link
Author

@timneutkens @wyattjoh How I said I had a lot of work to do. But now the MR should be ready to merge

@HelmerBarcos HelmerBarcos force-pushed the FIX_DEBUGGER_PORT branch 3 times, most recently from 894d346 to c2473a2 Compare October 25, 2023 20:46
@HelmerBarcos HelmerBarcos changed the title fix(debugger): accept arbitrary ip an port as NODE_OPTIONS fix(debugger): accept arbitrary ip and port as NODE_OPTIONS Nov 1, 2023
@gaac510
Copy link

gaac510 commented Nov 7, 2023

Is there a version we can use that does not have the bug that is fixed in this PR?

Hope it's OK to ask this question again, since this PR appears to be held up again.

@HelmerBarcos
Copy link
Author

HelmerBarcos commented Nov 9, 2023

Any updates for this PR?

@timneutkens timneutkens added the CI approved Approve running CI for fork label Nov 12, 2023
@ijjk
Copy link
Member

ijjk commented Nov 12, 2023

Allow CI Workflow Run

  • approve CI run for commit: c2473a2

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

1 similar comment
@ijjk
Copy link
Member

ijjk commented Nov 12, 2023

Allow CI Workflow Run

  • approve CI run for commit: c2473a2

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@simonstaton
Copy link

simonstaton commented Nov 14, 2023

Any update on this? Currently it's impossible to debug a NextJS application running in a docker container.

@HelmerBarcos
Copy link
Author

Should I do anything? I don't see any approvals or changes requested after 1 month.

@mkoreo
Copy link

mkoreo commented Dec 2, 2023

@timneutkens Hey, could you have another look at this PR? I believe @HelmerBarcos has addressed the remarks.

@mark-davenport-fountain
Copy link

I didn't see this PR before opening a very similar one here - #59410

The only real difference I think is that I've also added ability to set the host. It didn't look like this is actually passing along the host to NODE_OPTIONS. I pulled the tests over from this PR to mine as well.

Not a concern to me which one gets merged, but it'd be nice to get this in so that we can have debugging supported in docker containers.

@HelmerBarcos
Copy link
Author

HelmerBarcos commented Jan 20, 2024

Im closing this PR and opening a new one. See #60919

@HelmerBarcos HelmerBarcos mentioned this pull request Jan 20, 2024
@github-actions github-actions bot added the locked label Feb 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork locked type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants