add internal documentation by aymen94 · Pull Request #26665 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation45 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Added internal documentation for contributors.

#26639

If it's accepted, I'll add more.

documentation is changed or added
commit message follows commit guidelines

Signed-off-by: Aymen Naghmouchi aymen.aymen@live.it

Seems OK to me, but here are the relevant questions for @nodejs/collaborators

This documents an internal-use flag but does it outside of the API documentation (so it won't appear on the nodejs.org website, for example). It's still a public document though, available on GitHub and in the source code. Do we want to document internal-use flags? Or is it actually best to just not have them documented anywhere at all because doing so will invariably encourage people to use them, which is exactly what we don't want? @bnoordhuis
We already have internal docs in doc/guides. I dislike that directory name and maybe we can change it some time. But for now, does it make sense to split this into another directory like it is here (doc/internal) perhaps with the hope of moving everything there eventually? Or should we keep it with everything else in doc/guides?
While it seems more-or-less OK to me (as in: better than not having it documented at all), I feel like the documentation for this flag is not quite correct, or at least could stand some significant improvements that wouldn't be that big of a problem to add. It should mention bootstrap code, no? @danbev @joyeecheung

If it's internal-only it's typically only used in tests right? If so, it might be better to place it in the tests documentation?

Thanks for adding more docs for internals!

@Trott I think for now, doc/guides is the best place for this.

Choose a reason for hiding this comment

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

Can you move this into doc/guides? I guess then a name like internal-flags.md (if we focus this document on flags) or developing-core.md/debugging-internals.md would be more suitable.

Co-Authored-By: aymen94 aymen94@users.noreply.github.com

@Trott Like @joyeecheung said I think the place for this is in doc/guides which was suggested in the original pr.

@danbev There are other undocumented things besides this --inspect-brk-node.
Is my opinion: I think we should add an internal doc, Also to remember that there are in the code.
@Trott @joyeecheung

@aymen94 There already are several internal docs under doc/guides, this just adds some more for the CLI flags, so it should be there as well.

Why not just actually document our CLI options? I can never remember the name of --expose-internal and every time I grep the test source to find its name I ask myself "why?". If we don't want to be pinned down on the feature, call it experimental.

If this CLI option, which I've never heard about (probably because its not documented!) does what it sounds like, I wish I'd know about it before.

Wrt. the text proposed here, I think the option allows debugging the javascript js built into node during its initial evaluation, right? That isn't clear enough, I think, because I'm not absolutely certain that's what its for.

Choose a reason for hiding this comment

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

Don't want to be the naysayer here, but the downsides of having extensive internal documentation in my opinion:

It adds to the time and effort required for making code changes. It is one thing to document the changes to the public API, and a completely different thing altogether to ask people to document the internals. Most contributors would either end up overextending themselves or not reaching some vague undocumented standards.
As rightly pointed out by @Trott, this would inevitably lead to the ecosystem getting a know a lot about the internals (read: implementation details) of Node.js. Since we've set a terrible precedent regarding breaking code that relies on implementation details or experimental features (I've have to deal with it myself while working on module: use compileFunction over Module.wrap #21573, for example), I'm afraid this might end up revealing too much and it might significantly affect our ability to improve the codebase unless we explicitly agree that it's fine to change anything that's not a part of our public API, even if it might break someone's code.

@ryzokuken If not traced, many contributors will forget or do not know that there are.

Maybe we should rename --inspect-brk-node? It's not obvious at all that it's not intended for general use.

@ryzokuken If not traced, many contributors will forget or do not know that there are.

Contributors will forget that certain options exist? I think that seems unlikely.

@ryzokuken It's for new contributors like me?
Only looking at the source code, I see that they are there.

I can never remember the name of --expose-internal and every time I grep the test source to find its name

I'm the same but have started using bash completion (--completion-bash flag) which I think helps. You might not be using bash but though I'd mention it just in case.

-->

Activate inspector on `host:port` and break at start of the first internal
JavaScript script executed when the inspector is available.

Choose a reason for hiding this comment

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

Double script sounds redundant to me. Perhaps, JavaScript code?

Choose a reason for hiding this comment

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

statement is probably more accurate (it won't break on the first declaration).

I agree with most of the points mentioned out by @Trott . Wanted to give a few more ideas like if we agree on creating these documentation, then we can open up a issue (should be a help-wanted and good-first-issue) so that we can get contribution(like this one), which will eventually grow and cover all these internal used stuffs. Then, we can add those documentation in new contributions list or somewhere, so that new people coming to read our source code can get benefited by. I guess its a win-win situation.

@danbev I use zsh, but bash things often work, I'll give it a try, thanks.

As rightly pointed out by @Trott, this would inevitably lead to the ecosystem getting a know a lot about the internals (read: implementation details) of Node.js.

I don't think adding internal documentations really makes a difference here. For people who want to hack internals, they'll be able to find out about these things either by reading code or by reading blog posts etc. (TBH our doc folder may be the last place they'll ever look at). If we really want to hide all the internals, we might as well never write explanatory comments in the code, but that hardly benefits anyone. We just need to be very clear that these are not features, but helpers for debugging and are likely to go away without any prior notice.

Either way, It'll probably take some time before documentation like this show up in significant places of the search results, if it ever gets there - if you don't know about the keywords, it's probably hard to find out about the document, but if you already do, then you may already have a better source of knowledge than this document.

Lots of people have commented, and there's been debate about details like whether this documentation should go in one directory or another, but no Collaborator has come on with a strong +1 or -1 just for the general concept of documenting our internal-use-only flags someplace for ourselves. Where do individual Collaborators stand on that?

(I would interpret joyeecheung as being +1, ryzokuken as being -1, danbev as +1, etc., but those are inferences on my part and I could very well be wrong. I'm asking people to be explicit.)

I don't feel strongly about this, I'm fine either with documenting this and fine if we don't.

I am +1, but I think it should be in doc/guides, or it should be together with other documents under doc/guides (the folder can be renamed, not necessarily in this PR)

I'm +1 as well. If we are afraid that people might mistake it for general documentation, we can add a clear disclaimer at the top of the document.

When reading the comments it sounds like the common opinion is to add the documentation, but only to doc/guides. @aymen94 would you mind moving the file accordingly and to add a disclaimer as suggested?

Signed-off-by: Aymen Naghmouchi aymen.aymen@live.it

This was referenced

Mar 11, 2021

This was referenced

May 25, 2021

This was referenced

Apr 7, 2022

This was referenced

Jan 31, 2023

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

doc

Issues and PRs related to the documentations.