src: add NODE_OPTIONS_STANDALONE #59693
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59693 +/- ##
==========================================
- Coverage 89.91% 89.91% -0.01%
==========================================
Files 667 667
Lines 196600 196792 +192
Branches 38594 38411 -183
==========================================
+ Hits 176766 176938 +172
- Misses 12270 12306 +36
+ Partials 7564 7548 -16
🚀 New features to boost your workflow:
|
|
How does deno consume the |
|
@legendecas argparse.cc is not part of the api, it's just code I want to turn into a test. deno would consume node_options.cc/h. |
|
Will deno be linked to libnode? |
|
no, just to the node options parser. |
| } | ||
| } | ||
|
|
||
| node::HandleEnvOptions(cli_options.per_isolate->per_env, |
There was a problem hiding this comment.
How would this be maintained going forward? For example, this creates a dependence on the current shape of PerProcessOptions etc, which are very much just ad-hoc internal data structures that are very subject to change. Things like HandleEnvOptions or Parse are also subject to internal refactoring, like for example adding a different mechanism than HandleEnvOptions for creating env var/flag pairs like EnvImplies("NODE_PENDING_DEPRECATION", "--pending-deprecation") and then just create inline versions for different parsing code to reduce the use of lambdas then this would be ditched. We can also change the option categories like creating RealmOptions etc, or stop using std::strings but use std::string_view on a backing store.
There was a problem hiding this comment.
i would not expect node to maintain any compatibility for this interface aside from somehow being able to compile it as a standalone file or set of files that doesn't need to link against uv/libnode/v8/etc.
There was a problem hiding this comment.
also if node decided at some point in the future that it was too annoying to maintain even that and removed the ifdefs or whatever it was, that would be understandable.
There was a problem hiding this comment.
If compatibility is not a concern, then I think it makes more sense to just do a CI elsewhere instead of checking the test into the Node.js source, because then the maintainability burden would be shifted to Node.js contributors, and may create barriers to new contributors if they don't know the context and somehow think that breaking or removing this test is bad (think how many tests that are created > 10 years ago tend to give us a hard time in the CI because nobody knows what they are doing anymore or whether they can be removed because they are just irrelevant now). We can keep the macros internally to reduce the merge conflicts, but keeping the test in the CI doesn't seem very helpful if it's supposed to be maintained by another project. (I think every time I try to touch the node.h I have a lot of doubts about whether I am breaking something important, even with some GitHub archeology I still am doubtful and as a result, I think a lot of public APIs there are just rotting without users because nobody is certain whether they can be touched)
There was a problem hiding this comment.
I'm think it is not guaranteed that node_options.h/cc has no dependencies like uv/libnode/v8/etc.
There was a problem hiding this comment.
Yeah I feel that to address #46339 we might end up adding a v8 dependency to the option parser....
I'd like to, in deno, parse command line arguments the exact same way that node does. It seems to me that the best way to achieve this is to simply use the same code. In order to do this as non-invasively as possible, I tried to just insert a few targeted
NODE_OPTIONS_STANDALONEchecks. I verified this using the below command, but ideally I'd like to add a test to the CI that this actually compiles (and maybe even run the resulting binary?)So if y'all would be interested in accepting this PR, I'd appreciate some pointers on the best way to make
argparse.ccbe a test.