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

[v16.x backport] src: disambiguate terms used to refer to builtins and addons #45663

Closed

Conversation

mhdawson
Copy link
Member

The term "native module" dates back to some of the oldest code in the code base. Within the context of Node.js core it usually refers to modules that are native to Node.js (e.g. fs, http), but it can cause confusion for people who don't work on this part of the code base, as "native module" can also refer to native addons - which is even the case in some of the API docs and error messages.

This patch tries to make the usage of these terms more consistent. Now within the context of Node.js core:

  • JavaScript scripts that are built-in to Node.js are now referred to as "built-in(s)". If they are available as modules, they can also be referred to as "built-in module(s)".
  • Dynamically-linked shared objects that are loaded into the Node.js processes are referred to as "addons".

We will try to avoid using the term "native modules" because it could be ambiguous.

Changes in this patch:

File names:

  • node_native_module.h -> node_builtins.h,
  • node_native_module.cc -> node_builtins.cc

C++ binding names:

  • native_module -> builtins

node::Environment:

  • native_modules_without_cache -> builtins_without_cache
  • native_modules_with_cache -> builtins_with_cache
  • native_modules_in_snapshot -> builtins_in_cache
  • native_module_require -> builtin_module_require

node::EnvSerializeInfo:

  • native_modules -> `builtins

node::native_module::NativeModuleLoader:

  • native_module namespace -> builtins namespace
  • NativeModuleLoader -> BuiltinLoader
  • NativeModuleRecordMap -> BuiltinSourceMap
  • NativeModuleCacheMap -> BuiltinCodeCacheMap
  • ModuleIds -> BuiltinIds
  • ModuleCategories -> BuiltinCategories
  • LoadBuiltinModuleSource -> LoadBuiltinSource

loader.js:

  • NativeModule -> BuiltinModule (the NativeModule name used in process.moduleLoadList is kept for compatibility)

And other clarifications in the documentation and comments.

PR-URL: #44135
Fixes: #44036
Reviewed-By: Jacob Smith [email protected]
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: Richard Lau [email protected]
Reviewed-By: Jiawen Geng [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Mohammed Keyvanzadeh [email protected]
Reviewed-By: Tobias Nießen [email protected]
Reviewed-By: Jan Krems [email protected]

The term "native module" dates back to some of the oldest code
in the code base. Within the context of Node.js core it usually
refers to modules that are native to Node.js (e.g. fs, http),
but it can cause confusion for people who don't work on this
part of the code base, as "native module" can also refer to
native addons - which is even the case in some of the API
docs and error messages.

This patch tries to make the usage of these terms more consistent.
Now within the context of Node.js core:

- JavaScript scripts that are built-in to Node.js are now referred
  to as "built-in(s)". If they are available as modules,
  they can also be referred to as "built-in module(s)".
- Dynamically-linked shared objects that are loaded into
  the Node.js processes are referred to as "addons".

We will try to avoid using the term "native modules" because it could
be ambiguous.

Changes in this patch:

File names:
- node_native_module.h -> node_builtins.h,
- node_native_module.cc -> node_builtins.cc

C++ binding names:
- `native_module` -> `builtins`

`node::Environment`:
- `native_modules_without_cache` -> `builtins_without_cache`
- `native_modules_with_cache` -> `builtins_with_cache`
- `native_modules_in_snapshot` -> `builtins_in_cache`
- `native_module_require` -> `builtin_module_require`

`node::EnvSerializeInfo`:
- `native_modules` -> `builtins

`node::native_module::NativeModuleLoader`:
- `native_module` namespace -> `builtins` namespace
- `NativeModuleLoader` -> `BuiltinLoader`
- `NativeModuleRecordMap` -> `BuiltinSourceMap`
- `NativeModuleCacheMap` -> `BuiltinCodeCacheMap`
- `ModuleIds` -> `BuiltinIds`
- `ModuleCategories` -> `BuiltinCategories`
- `LoadBuiltinModuleSource` -> `LoadBuiltinSource`

`loader.js`:
- `NativeModule` -> `BuiltinModule` (the `NativeModule` name used in
  `process.moduleLoadList` is kept for compatibility)

And other clarifications in the documentation and comments.

PR-URL: nodejs#44135
Fixes: nodejs#44036
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp
  • @nodejs/modules
  • @nodejs/node-api
  • @nodejs/startup
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v16.x labels Nov 28, 2022
@mhdawson
Copy link
Member Author

mhdawson commented Nov 28, 2022

Did this backport to unlock backporting of #44376

This was not an easy backport, many conflicts and there are several PRs that are marked as don't land on 16.x on which this depends. This meant that additional/different files like node_builtins_env.cc needed to be updated. Quite a lot of manual work so needs a good review.

One key question which we should confirm is that changing the name of the builtin is SemVer patch as indicated on the original PR. I assume it is but @joyeecheung could you confirm?

mhdawson added a commit to mhdawson/io.js that referenced this pull request Nov 28, 2022
Found while backporting
nodejs#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>
@mscdex mscdex changed the title src: disambiguate terms used to refer to builtins and addons [v16.x backport] src: disambiguate terms used to refer to builtins and addons Nov 29, 2022
mhdawson added a commit that referenced this pull request Dec 2, 2022
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

richardlau pushed a commit that referenced this pull request Dec 8, 2022
The term "native module" dates back to some of the oldest code
in the code base. Within the context of Node.js core it usually
refers to modules that are native to Node.js (e.g. fs, http),
but it can cause confusion for people who don't work on this
part of the code base, as "native module" can also refer to
native addons - which is even the case in some of the API
docs and error messages.

This patch tries to make the usage of these terms more consistent.
Now within the context of Node.js core:

- JavaScript scripts that are built-in to Node.js are now referred
  to as "built-in(s)". If they are available as modules,
  they can also be referred to as "built-in module(s)".
- Dynamically-linked shared objects that are loaded into
  the Node.js processes are referred to as "addons".

We will try to avoid using the term "native modules" because it could
be ambiguous.

Changes in this patch:

File names:
- node_native_module.h -> node_builtins.h,
- node_native_module.cc -> node_builtins.cc

C++ binding names:
- `native_module` -> `builtins`

`node::Environment`:
- `native_modules_without_cache` -> `builtins_without_cache`
- `native_modules_with_cache` -> `builtins_with_cache`
- `native_modules_in_snapshot` -> `builtins_in_cache`
- `native_module_require` -> `builtin_module_require`

`node::EnvSerializeInfo`:
- `native_modules` -> `builtins

`node::native_module::NativeModuleLoader`:
- `native_module` namespace -> `builtins` namespace
- `NativeModuleLoader` -> `BuiltinLoader`
- `NativeModuleRecordMap` -> `BuiltinSourceMap`
- `NativeModuleCacheMap` -> `BuiltinCodeCacheMap`
- `ModuleIds` -> `BuiltinIds`
- `ModuleCategories` -> `BuiltinCategories`
- `LoadBuiltinModuleSource` -> `LoadBuiltinSource`

`loader.js`:
- `NativeModule` -> `BuiltinModule` (the `NativeModule` name used in
  `process.moduleLoadList` is kept for compatibility)

And other clarifications in the documentation and comments.

PR-URL: #44135
Backport-PR-URL: #45663
Fixes: #44036
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@richardlau
Copy link
Member

Landed in fbc52e5.

@richardlau richardlau closed this Dec 8, 2022
targos pushed a commit that referenced this pull request Dec 12, 2022
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
The term "native module" dates back to some of the oldest code
in the code base. Within the context of Node.js core it usually
refers to modules that are native to Node.js (e.g. fs, http),
but it can cause confusion for people who don't work on this
part of the code base, as "native module" can also refer to
native addons - which is even the case in some of the API
docs and error messages.

This patch tries to make the usage of these terms more consistent.
Now within the context of Node.js core:

- JavaScript scripts that are built-in to Node.js are now referred
  to as "built-in(s)". If they are available as modules,
  they can also be referred to as "built-in module(s)".
- Dynamically-linked shared objects that are loaded into
  the Node.js processes are referred to as "addons".

We will try to avoid using the term "native modules" because it could
be ambiguous.

Changes in this patch:

File names:
- node_native_module.h -> node_builtins.h,
- node_native_module.cc -> node_builtins.cc

C++ binding names:
- `native_module` -> `builtins`

`node::Environment`:
- `native_modules_without_cache` -> `builtins_without_cache`
- `native_modules_with_cache` -> `builtins_with_cache`
- `native_modules_in_snapshot` -> `builtins_in_cache`
- `native_module_require` -> `builtin_module_require`

`node::EnvSerializeInfo`:
- `native_modules` -> `builtins

`node::native_module::NativeModuleLoader`:
- `native_module` namespace -> `builtins` namespace
- `NativeModuleLoader` -> `BuiltinLoader`
- `NativeModuleRecordMap` -> `BuiltinSourceMap`
- `NativeModuleCacheMap` -> `BuiltinCodeCacheMap`
- `ModuleIds` -> `BuiltinIds`
- `ModuleCategories` -> `BuiltinCategories`
- `LoadBuiltinModuleSource` -> `LoadBuiltinSource`

`loader.js`:
- `NativeModule` -> `BuiltinModule` (the `NativeModule` name used in
  `process.moduleLoadList` is kept for compatibility)

And other clarifications in the documentation and comments.

PR-URL: nodejs/node#44135
Backport-PR-URL: nodejs/node#45663
Fixes: nodejs/node#44036
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
The term "native module" dates back to some of the oldest code
in the code base. Within the context of Node.js core it usually
refers to modules that are native to Node.js (e.g. fs, http),
but it can cause confusion for people who don't work on this
part of the code base, as "native module" can also refer to
native addons - which is even the case in some of the API
docs and error messages.

This patch tries to make the usage of these terms more consistent.
Now within the context of Node.js core:

- JavaScript scripts that are built-in to Node.js are now referred
  to as "built-in(s)". If they are available as modules,
  they can also be referred to as "built-in module(s)".
- Dynamically-linked shared objects that are loaded into
  the Node.js processes are referred to as "addons".

We will try to avoid using the term "native modules" because it could
be ambiguous.

Changes in this patch:

File names:
- node_native_module.h -> node_builtins.h,
- node_native_module.cc -> node_builtins.cc

C++ binding names:
- `native_module` -> `builtins`

`node::Environment`:
- `native_modules_without_cache` -> `builtins_without_cache`
- `native_modules_with_cache` -> `builtins_with_cache`
- `native_modules_in_snapshot` -> `builtins_in_cache`
- `native_module_require` -> `builtin_module_require`

`node::EnvSerializeInfo`:
- `native_modules` -> `builtins

`node::native_module::NativeModuleLoader`:
- `native_module` namespace -> `builtins` namespace
- `NativeModuleLoader` -> `BuiltinLoader`
- `NativeModuleRecordMap` -> `BuiltinSourceMap`
- `NativeModuleCacheMap` -> `BuiltinCodeCacheMap`
- `ModuleIds` -> `BuiltinIds`
- `ModuleCategories` -> `BuiltinCategories`
- `LoadBuiltinModuleSource` -> `LoadBuiltinSource`

`loader.js`:
- `NativeModule` -> `BuiltinModule` (the `NativeModule` name used in
  `process.moduleLoadList` is kept for compatibility)

And other clarifications in the documentation and comments.

PR-URL: nodejs/node#44135
Backport-PR-URL: nodejs/node#45663
Fixes: nodejs/node#44036
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
Found while backporting
#45663

Fixup one rename missed

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45665
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants