src: make debug_options getters public - #30494
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
This has me scratching my head a lot…
I think this is the main issue here – we should get Electron away from having to do this manually, and provide an API for that, rather than exposing various internals. In #30467, inspector setup would be moved into LoadEnvironment() (i.e. part of a public API), so it would be awesome if you could take a look at that, both from a general API perspective and regarding how it affects Electron specifically.
I’m a bit confused by this – src/node_options.h and src/node_options-inl.h should both be available to Electron in the same way, and neither of them are public-facing (anything guarded by NODE_WANT_INTERNALS is private), so I’m surprised that this PR improves the situation for Electron? I feel like learning a bit more about why and how it does that might help me understand more about how Electron uses Node’s source tree. |
Sorry, something went wrong.
|
@addaleax since debug_options was defined in src/node_options-inl.h we'd need to require an extra file to do what could trivially be consolidated into one. This just simplifies our requires, since we already require the other file. |
Sorry, something went wrong.
|
@codebytere Going by that, it’s probably the simplest solution if you only include the -inl.h file? But yeah, you can feel free to go ahead and land this – just maybe update the commit message so it doesn’t give the impression that something’s being made part of the public API? |
Sorry, something went wrong.
|
Will do! thanks :) |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
This simplifies requires for those using DebugOptions, since debug_options was defined in src/node_options-inl.h and thus embedders would need to require an extra file to do what could trivially be consolidated into one. PR-URL: #30494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>
|
Landed in ccdd6ef |
Sorry, something went wrong.
This simplifies requires for those using DebugOptions, since debug_options was defined in src/node_options-inl.h and thus embedders would need to require an extra file to do what could trivially be consolidated into one. PR-URL: #30494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>
This simplifies requires for those using DebugOptions, since debug_options was defined in src/node_options-inl.h and thus embedders would need to require an extra file to do what could trivially be consolidated into one. PR-URL: #30494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>
This simplifies requires for those using DebugOptions, since debug_options was defined in src/node_options-inl.h and thus embedders would need to require an extra file to do what could trivially be consolidated into one. PR-URL: #30494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>
Refs #30466.
Now that Electron is parsing and passing CLI options, we can set DebugOptions ourselves through ParseGlobalArgs. However, we also need to create and start the inspector agent with the DebugOptions set by users, since the inspector agent Start function takes a parameter const DebugOptions& options.
The most straightforward way to do this is for us to pass env->options()->debug_options() to our inspector agent Start call, but since debug_options was defined in src/node_options-inl.h we'd need to require extra files to do what could trivially be consolidated into one. This PR therefore moves those definitions into a public-facing file, allowing us to pass correct options to the inspector without needing to parse all-new ones we'd previously parsed on startup.
cc @joyeecheung @addaleax
Checklist