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

add internal documentation #26665

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
@aymen94
Copy link
Member

commented Mar 14, 2019

Added internal documentation for contributors.

#26639

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

doc: add internal documentation
Signed-off-by: Aymen Naghmouchi <aymen.aymen@live.it>
@aymen94

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

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?

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Thanks for adding more docs for internals!

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

@joyeecheung
Copy link
Member

left a comment

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.

Show resolved Hide resolved doc/internal/index.md Outdated
Update doc/internal/index.md
Co-Authored-By: aymen94 <aymen94@users.noreply.github.com>
@danbev

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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

@aymen94

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@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

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@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.

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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.

@ryzokuken
Copy link
Member

left a comment

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

  1. 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.
  2. 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 #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.
@aymen94

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

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

@ryzokuken

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@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.

@aymen94

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

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

@danbev

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

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.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Mar 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 18, 2019

Member

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

@antsmartian

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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.

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

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

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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.

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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?

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

(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.)

@danbev

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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)

@targos

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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.

@addaleax addaleax referenced this pull request Mar 21, 2019

Closed

tls: add debugging to native TLS code #26843

2 of 2 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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?

change directory, add a disclaimer
Signed-off-by: Aymen Naghmouchi <aymen.aymen@live.it>
@@ -0,0 +1,17 @@
# we assume no responsibility

This comment has been minimized.

Copy link
@Trott

Trott Mar 25, 2019

Member

?

This comment has been minimized.

Copy link
@aymen94

aymen94 Mar 26, 2019

Author Member

what should I put as a disclaimer?

This comment has been minimized.

Copy link
@Trott

Trott Mar 26, 2019

Member

what should I put as a disclaimer?

Why does this file need a disclaimer?

This comment has been minimized.

Copy link
@aymen94

This comment has been minimized.

Copy link
@Trott

Trott Mar 26, 2019

Member

That's requesting a disclaimer along the lines of "These flags are for Node.js core development usage only. Do not use these flags in your own applications. These flags are not subjected to semantic versioning rules. The core developers may remove these flags in any version of Node.js."

This comment has been minimized.

Copy link
@Trott

Trott Mar 26, 2019

Member

(In other words, not a legal disclaimer of no liability, but a disclaimer to not use these flags if you're doing it for internal Node.js core development.)

change disclaimer
Signed-off-by: Aymen Naghmouchi <aymen.aymen@live.it>
Show resolved Hide resolved doc/guides/internal/index.md Outdated
@Trott
Copy link
Member

left a comment

Can you rename the file to something descriptive? index.md is pretty meaningless in this context--these are not files for our webserver or anything like that--and this shouldn't be the default file people get anyway. Maybe cli.md? Or cli-undocumented-flags.md?

remove span
Co-Authored-By: aymen94 <aymen94@users.noreply.github.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Thanks for your patience with all of this. Last set of nit-picks, I think. These are optional because someone else can do them later if they really want to. But I think it would marginally improve this doc:

  • Move the disclaimer from the top to underneath ### Node.js options:
  • Remove the colon at the end of ### Node.js options:
  • Since it's already under CLI, maybe change ### Node.js options: to ### Flags
@Trott

Trott approved these changes Mar 26, 2019

@richardlau

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Can you rename the file to something descriptive? index.md is pretty meaningless in this context--these are not files for our webserver or anything like that--and this shouldn't be the default file people get anyway. Maybe cli.md? Or cli-undocumented-flags.md?

I would also suggest adding a doc/guides/internal/README.md as GitHub will automatially display it if you browse to the directory in the Web UI (e.g. see the test directory). The README.md should contain a similar disclaimer that things documented in this directory are for internal use only.

aymen94 added some commits Mar 28, 2019

change file name to readme
Signed-off-by: Aymen Naghmouchi <aymen.aymen@live.it>
@Trott

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I guess we can also land it as is and change the rest later?

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@richardlau @Trott are you fine with landing this as is?

@richardlau

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@richardlau @Trott are you fine with landing this as is?

¯\_(ツ)_/¯ No objections if other collaborators find this useful.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@richardlau @Trott are you fine with landing this as is?

Yes. Preferable if comments are addressed, but not required.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 15, 2019

doc: add internal documentation
This adds documentation that is meant as internal only. As first entry
the `--inspect-brk-node` flag got documented here.

PR-URL: nodejs#26665
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Landed in 7938238 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.