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

doc: add glossary.md #27517

Closed
wants to merge 1 commit into from
Closed

doc: add glossary.md #27517

wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

Related issue: #26718.

Todo

I want to add primordials, but I have no idea what it means.

cc @refack @BridgeAR

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 1, 2019
@mscdex
Copy link
Contributor

mscdex commented May 1, 2019

Some of these terms are already defined in the linked Chromium glossary.

@Trott
Copy link
Member

Trott commented May 1, 2019

Can you move this under doc/?

@gengjiawen
Copy link
Member Author

Some of these terms are already defined in the linked Chromium glossary.

I do it deliberately because LGTM is too common.

@gengjiawen
Copy link
Member Author

Can you move this under doc/?

My thought is that it's very basic doc, so I would like to be in the root directory.
I am okay either way.

@gengjiawen
Copy link
Member Author

Do we need to add TSC to this ?

glossary.md Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

BridgeAR commented May 3, 2019

Do we need to add TSC to this ?

If we add a glossary, I'd say it should contain references to all acronyms we use.

My thought is that it's very basic doc, so I would like to be in the root directory.

I am on the edge about this. I see the benefit of having in the root dir but we already have to many files in there (we might want to move some other files like the CPP style guide. That does not seem to belong into the root dir. The collaborator guide could likely also be moved since it's intention is mainly for collaborators and not for everyone).

I want to add primordials, but I have no idea what it means.

Those are pristine built-ins that are not effected by prototype pollution and tampering with built-ins.

@refack
Copy link
Contributor

refack commented May 3, 2019

Can you move this under doc/?

My intuition is that this should be in https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md (prbly at the end)

@refack
Copy link
Contributor

refack commented May 3, 2019

Some more terms that come to mind, esp. when in the context of Node.js

  • bootstrap
  • code cache
  • snapshot
  • ESM / CJS / .cjs / .mjs
  • deps
  • vendoring
  • V8
  • VM
  • JS/C++ boundary
  • native modules/addons

@gengjiawen
Copy link
Member Author

gengjiawen commented May 3, 2019

Some more terms that come to mind, esp. when in the context of Node.js

  • bootstrap
  • code cache
  • snapshot
  • ESM / CJS / .cjs / .mjs
  • deps
  • vendoring
  • V8
  • VM
  • JS/C++ boundary
  • native modules/addons

Add links to this PR: #26929 ?

This is very a good idea. I wonder what we can do to get more people contribute to Node.js. @joyeecheung recently write a ppt, which is very good to read (Written in Chinese): https://github.com/joyeecheung/talks/blob/master/code_and_learn_2019_beijing/contributing-to-node-core.pdf.

@gengjiawen gengjiawen marked this pull request as ready for review May 8, 2019 08:23
@gengjiawen
Copy link
Member Author

@refack @BridgeAR Can you review this, thanks.

@jasnell
Copy link
Member

jasnell commented May 12, 2019

Not sure this needs to be in a separate document at the top level. This can be added as a section of the contributor guide.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The documentation itself is LGTM. I leave the decision about the location to others.

Trott
Trott previously requested changes May 13, 2019
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Terms should be in alphabetical order, I would think?

@gengjiawen
Copy link
Member Author

Not sure this needs to be in a separate document at the top level. This can be added as a section of the contributor guide.

It's more clear when it in one file such as chrome did.
Also this file may grow a little longer.

@gengjiawen
Copy link
Member Author

Is is possible to write a test for this new handler? I think you can just write some OOB in WAT, compile it to wasm, and load it in a child process and check SIGSEGV?

FROM #27246 (review)

So what's OOB and WAT ? Should we add those two. cc @joyeecheung

@gengjiawen
Copy link
Member Author

Terms should be in alphabetical order, I would think?

Now sorted. (Using vim sort)

@Trott
Copy link
Member

Trott commented May 16, 2019

Is is possible to write a test for this new handler? I think you can just write some OOB in WAT, compile it to wasm, and load it in a child process and check SIGSEGV?

FROM #27246 (review)

So what's OOB and WAT ? Should we add those two. cc @joyeecheung

I'd avoid adding that (and, probably, a glossary at all, to be honest, but I won't block if others think this is a good idea). These terms are not Node.js-specific. People can use search engines and/or we can link to a general glossary that has these sorts of things. We can't include every piece of information imaginable in our source code, and trying will make the important stuff harder to find. We need to curate our information. (Why not include handler? SIGSEGV? Wasm? Child process? Process? Web server? Test?)

Realistically, I suspect people aren't going to look in the glossary anyway. They're going to ask, "What is WAT?"

@Trott Trott dismissed their stale review May 16, 2019 14:57

entries alphabetized

@Trott
Copy link
Member

Trott commented May 16, 2019

I guess I'd be more inclined to support this if we either just linked to Chrome's glossary (why duplicate the work?) or held the glossary to be strictly Node.js-specific terms or things very closely tied to Node.js. None of these except maybe primordials is specific to Node.js.

@gireeshpunathil
Copy link
Member

IMO, the ones mentioned in #27517 (comment) are going to be really useful for new comers, irrespective of their sole origin is our project or not, (and in some case how trivial those may look), but those are heavily used across the code and doc. Here are some more:

ABI, ASAP, ASM, ASYNC, BE, CI, CITGM, CJS, CLDR, CLI, CMD, CVE, ECMA, ENV, EOF, EOL, ESM, ETW, FD, FFDC, FIPS, FS, ICU, IPC, JIT, LTS, MDN, OOB, OOM, OSX, PPC, RAII, REPL, RFC, RSS, RTTI, SMP, TSC, WASI, WASM, WG, WHATWG

glossary.md Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

glossary.md Outdated Show resolved Hide resolved
glossary.md Outdated Show resolved Hide resolved
glossary.md Outdated Show resolved Hide resolved
glossary.md Outdated Show resolved Hide resolved
glossary.md Outdated Show resolved Hide resolved
glossary.md Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the glossary branch 2 times, most recently from 5161e3e to f08236d Compare January 23, 2020 06:13
@nodejs-github-bot

This comment has been minimized.

@gengjiawen gengjiawen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@@ -0,0 +1,16 @@
You may also need to check https://chromium.googlesource.com/chromiumos/docs/+/master/glossary.md.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: shouldn't this go at the bottom of the file, as it's meant to be an addition to this file based in the text?

* PTAL: Please take a look.
* RSLGTM: "Rubber-stamp looks good to me". The reviewer approving without doing
a full code review.
* TSC: Technical Steering Committee. Detailed info see
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
* TSC: Technical Steering Committee. Detailed info see
* TSC: Technical Steering Committee. Detailed info can be found here

Or

Suggested change
* TSC: Technical Steering Committee. Detailed info see
* TSC: Technical Steering Committee. For detailed info see

* WIP: "Work In Progress" - e.g. a patch that's not finished, but may be worth
an early look.
* WPT: [web-platform-tests](https://github.com/web-platform-tests/wpt)
* godbolt: [Compiler Explorer](https://godbolt.org/) run compilers interactively
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
* godbolt: [Compiler Explorer](https://godbolt.org/) run compilers interactively
* godbolt: [Compiler Explorer](https://godbolt.org/) allows to run compilers interactively

gengjiawen added a commit that referenced this pull request Feb 14, 2020
PR-URL: #27517
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@gengjiawen
Copy link
Member Author

Landed in 13c05cd

@gengjiawen gengjiawen closed this Feb 14, 2020
@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 14, 2020

@lundibundi I will open a pr to resolve this and add more items.

@gengjiawen gengjiawen reopened this Feb 14, 2020
@gengjiawen gengjiawen closed this Feb 14, 2020
@gengjiawen gengjiawen deleted the glossary branch February 14, 2020 03:30
codebytere pushed a commit that referenced this pull request Feb 17, 2020
PR-URL: #27517
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
PR-URL: #27517
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
PR-URL: #27517
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
PR-URL: #27517
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants