-
Notifications
You must be signed in to change notification settings - Fork 7
feat(generator): add node.config.json
#253
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
Conversation
ack! This contains a commit from #252 |
node.config.json
d8da28b
to
5518b19
Compare
5518b19
to
f9c1321
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, I don't necessarily like using regex here vs what it done in core (using getEnvOptionsInputType
), but I can't think of anything better unless we want to require the --expose-internals
flag to run this generator but that's kinda awkward
We couldn't really use the |
@nodejs/config to verify this schema |
Im not sure I understand the goal of this PR, the schema needs to be generated by the node.js version for which the schema is meant |
This PR aims to migrate schema generation into the As demonstrated in this PR, the schema can be generated using a different Node.js version, as long as the files can be parsed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regexing on a .cc file is not a good idea, I dont see the benefit, there is an internal api designated for this specific purpose
@marco-ippolito The internal API requires that the schema to be generated using the same Node.js version it’s intended for. Ideally, this generator should support generating schemas with previous versions—specifically the LTS versions used in CI. Do you have an alternative method you recommend? |
Whats wrong with the current way of generating the schema? It seems to be working fine |
There's nothing "wrong" with it, however, the schema must be generated by the same version of Node.js that it is being generated for, which isn't consistent with the rest of |
The schema is generated at release time just like api docs. Id rather have this costraint than regex parse a .cc file |
I respectfully disagree, @marco-ippolito. The new API documentation tooling allows you to use it with different versions of Node.js, independent of specific Node source code. If we want to generate Additionally, your block has no authority over this repository. Could you please elaborate on your statement concerning "Regexing on a .cc file is not a good idea; I don’t see the benefit." We already use regex on source files for various features in the API documentation tooling generation, so I’m unclear about why you oppose this particular case in both the old and new tooling. |
This repository falls under Web Infra governance; therefore, a Core Collaborator block does not apply here. Although I do believe consensus seeking should be key here.
This comment was marked as resolved.
This comment was marked as resolved.
this is new to me, since when I am not authorized to block a PR as a collaborator @nodejs/tsc ? Since I created the |
@ovflowd can you point to me to the doc saying I cannot block a PR in the doc tooling repo? |
Marco is a TSC member, so based on your words, it's okay for him to block this PR. |
This comment was marked as resolved.
This comment was marked as resolved.
So this group policy is that a collaborator review can be dismissed without discussion?
You could have asked me to elaborate more but you went ahead and removed my review I have no interest in interacting with this PR anymore, since its clear my opinion is not respected. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
That is true for Working Groups but not teams. |
Noted! Apologies, again @marco-ippolito, as my actions seem to have been misplaced. I understand you don't want to interact with this PR anymore, but as we are sunsetting the existing api-doc-tooling, we need some guidance on how to proceed with the node.config generation. Would you be so kind to guide us here? |
My 2 cents on the objection dismissal. First of all, Claudio, let me clarify I totally believe in good faith. Whether Marco was or was not a TSC member was not the point, but rather than I also felt like his opinion was totally ignored with the dismissal. Being a TSC doesn't make his opinion more or less important. It's just about the person. My (unsolicited) advice for the next time is eventually pointing out in a comment that the objection might not have been binding according to the rules. Then, after thoroughly have gone for consensus seeking as we usually do, eventually override the objection and move on with the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking the PR on behalf of @marco-ippolito as me dismissing their reviews was not an accurate representation of the governance docs and it unnecessarily dismissed Marco's opinions which still hold true.
|
||
// Regex pattern to match calls to the AddOption function. | ||
export const ADD_OPTION_REGEX = | ||
/AddOption[\s\n\r]*\([\s\n\r]*"([^"]+)"(.*?)\);/gs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that this parses the C++ source code to get the options? If so this feels like a misstep. The current C++ option parser puts an overhead at runtime to add the options. Ideally that should be refactored to add most of the options at compile time and let the parser be statically generated to eliminate the overhead and eliminate more static STL containers. Parsing the source code in an ad-hoc way in another tooling that would break the tests makes it harder to change the parser in the future, the more we do it the more the doc tooling can become a road block on performance optimisations of core which doesn't sound right. It would be better to use the binary to get the options instead of counting on the source code to be a particular shape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joyeecheung for the feedback! I feel the straightforward way is to simply use the existing code incorporated on this repository? Or should the generation of the node.config.json
schema simply not be handled by the api-docs-tooling
🤔 any thoughts or alternatives are appreciated 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in turns of churn/volatility, using public APIs is better than using internal APIs, and using internal APIs is better than parsing the source. It might warrant a feature request for a public API of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Joyee, that feels like a good path forward.
@avivkeller should we maybe close this PR and opt-in for a very simple generator that simply imports https://github.com/nodejs/node/blob/2244a0977451c16e7cade9f1aeb46e1c136669bb/tools/doc/generate-json-schema.mjs#L4 and generates it? I worry that there might be an alternative here without necessarily exposing Node internals, or if the generation of this file should be handled elsewhere? Any guidance would be super helpful! |
Hey folks 👋 I've chatted with @marco-ippolito and we reached on the following:
@avivkeller how do you feel with this? Does it sound like a good path to proceed? |
Sounds good to me! If relevant, you may also want to close/update #216 with the additional information |
For uploading it to |
Description
This generates the https://nodejs.org/node-config-schema.json file (https://raw.githubusercontent.com/nodejs/node/main/doc/node-config-schema.json).
Validation
Other than this schema including
experimental-quic
(which the Node core one does not), it should be a 1:1 match with the generator in Node core (This is likely due to the machine generating the node core schema not having quic).Example Output
Related Issues
Fixes #216
Check List