src: move public C++ APIs into src/api/*.cc

[joyeecheung]

src: move v8_platform implementation into node_v8_platform-inl.h

So that the v8_platform global variable can be referenced in other
files.

src: move public C++ APIs into src/api/*.cc

This patch moves most of the public C++ APIs into src/api/*.cc
so that it's easier to tell that we need to be careful about
the compatibility of these code.

Some APIs, like node::LoadEnvironmet(), node::Start() and
node::Init() still stay in node.cc because they are still
very specific to our use cases and do not work quite well yet
for embedders anyway - we could not even manage to write cctest for
them at the moment.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2270/pipeline

[joyeecheung]

CI: https://ci.nodejs.org/job/node-test-pull-request/20170/

[sam-github]

nit: Some APIs, like node::LoadEnvironmet() in commit message, "Environment" spelling

[sam-github]

What is our API? Is it everything visible to #include <node.h>? I wouldn't have thought signo_string() was part of our API.

Breaking the node.cc monolith up a bit is great, but what's the criteria for including/excluding code from src/api/*.cc?

It should be stated clearly enough (in a file in src or src/api) that independent reviewers can come to the same conclusion on whether code belongs in src/api or just src/, otherwise the distinction will rot with time, and the creating of a new sub-dir will come to be seen as an accident of history.

[bnoordhuis]

I wouldn't have thought signo_string() was part of our API.

It's a remnant from when node was still young and undisciplined.

[joyeecheung]

What is our API? Is it everything visible to #include <node.h>?

Yes. No matter we want it or not they are already in the header so are considered "documented".

what's the criteria for including/excluding code from src/api/*.cc?

For this PR my criteria was: everything visible in "node.h" except those that cannot even be tested by us (e.g. node::LoadEnvironment(), node::Start() and node::Init()) - that means they are simply broken for most use cases that are not the main node executable. When we fix them, it's very likely that we will create a new implementation under src/api instead of reusing the code in node.cc, so there is not a lot of point moving that node-executable-specific code to src/api now if the majority of that will be moved back later.

I also left out the addon loading code because I want to focus this PR mostly on code moves, but I plan to move the common bits (node_module_register) into src/api later and reuse it from node_binding.cc - if that's part of the public API, we should make that more obvious.

[joyeecheung]

It should be stated clearly enough (in a file in src or src/api)

I can document that in the C++ style guide but I'd prefer we do not document it in src now - we do not have any .md in either src or lib, it feels odd to document nothing else but this contract there, but it also takes too much extra effort to document the whole directory structure.

[sam-github]

@joyeecheung I'm not suggesting reams of formal docs. I suggest a prominent comment in node.h to the effect that anything visible from including <node.h> is visible to addons, and is therefore part of our external API. It should be implemented in src/api/, and treated with great care.

IMO, its a bit odd that node.h (since it defines our API) is not itself in api/, along with all the headers it includes. Putting all the api headers in a place so they are clearly seen as our API, rather being mixed in with the various internal headers, would be at least as useful as moving the source, maybe more so. It also makes review easy, if an include of a .h file NOT in api/ was ever added to a header in api/, it would be obvious that this is an inadvertent API addition.

[joyeecheung]

@sam-github I don't quite understand the request, why would we want to put node.h under src/api and put a comment about the implementation details in node.h? (It does make sense to add a comment about everything in node.h is public, though). In most C++ projects the heades are usually under some folder like include and the implementation should be in somewhere like src, and I don't think I've seen public headers that document where the sources are - it's like documenting lib/internal in our API docs.

Moving the implementation to src/api/ is not necessarily related to this convention - the purpose is to make it more obvious that the implementations should be careful about compatibility.

IMO, its a bit odd that node.h (since it defines our API) is not itself in api/, along with all the headers it includes.

I don't think we would want to add any more includes in node.h.

It also makes review easy, if an include of a .h file NOT in api/ was ever added to a header in api/, it would be obvious that this is an inadvertent API addition.

Putting all the api headers in a place so they are clearly seen as our API, rather being mixed in with the various internal headers

There are only two public headers, node.h and node_version.h. The v8 headers are not owned by us. Internal headers are all guarded with NODE_WANT_INTERNALS - it's all pretty simple so I don't think the current layout is particularly problematic? (but sure we could use more documentations) However scattering implementations in different files under src makes it easy to accidentally break an API without noticing that it's exported in node.h, particularly when we do not have a lot of cctests of our own.

[sam-github]

In most C++ projects the heades are usually under some folder like include and the implementation should be in somewhere like src

Yes, that's common, but node doesn't do that.

I think if you don't write down somewhere what src/api is for so that future devs will understand clearly, then the distinction won't be kept. node.h was only one suggestion, the top of every .cc file in src/api is another, and I don't see the problem with a 3 line text file in src/api either. Or you can not document and see how it goes, but "api" is pretty generic. Many aspects of the node javascript API are implemented in C++, a reasonable person might conclude the C++ that implements the node JS API should be in src/api. Maybe "node" would make more sense, if its the implementation of whatever was declared in "node.h".

In short, you've been doing lots of reorg in src/, I'm not objecting to this, just commenting. I trust this reorg makes sense to you, but it wasn't (and still isn't) clear to me why its so beneficial to have a new directory. I'm having to guess the motivation.

It looks to me is that you want to break up node.cc, because its a giant mix of way too many different kinds of things? having src/node_encoding.cc, src/node_callback.cc would do that as well without introducing a new sub-dir.

[joyeecheung]

In short, you've been doing lots of reorg in src/, I'm not objecting to this, just commenting. I trust this reorg makes sense to you, but it wasn't (and still isn't) clear to me why its so beneficial to have a new directory. I'm having to guess the motivation.

The benefit is at least it's obvious to people who realize what src/api is for to notice that they are changing part of the public API when they touch these files and therefore be careful about the compatibility. The remaining code not being moved in this PR are the binding code (which I think can be moved in another PR later because it involves a bit more refactoring), and
APIs that do not really work like node::LoadEnvironment(), node::Start() and node::Init() (they are not really usable at the moment so compatibility is not a primary concern).

Many aspects of the node javascript API are implemented in C++, a reasonable person might conclude the C++ that implements the node JS API should be in src/api.

That should be addressed in documentations about the general directory structure, we could go anywhere with plain guessing. For people who are aware that src is C++ and lib is JS, I think it's not too difficult to guess that src contains C++ APIs. For people who are not aware of that distinction, the naming and the number of the src/api files probably won't lead them to that conclusion.

It looks to me is that you want to break up node.cc, because its a giant mix of way too many different kinds of things? having src/node_encoding.cc, src/node_callback.cc would do that as well without introducing a new sub-dir.

That was my intention, moving them to src/api was just to make it more obvious that these implementations are public.

[sam-github]

For people who are aware that src is C++ and lib is JS

I wasn't suggesting that any reasonable person would think src/ contains javascript. Some of the C++ in src/ implements methods directly callable from javascript, with APIs documented in our API docs, but that C++ is not src/api/, just the C++ addons API is. Maybe api should be named addon-api, I think that would be clearer, and probably wouldn't even need documentation.

src contains C++ APIs

If src/ contains C++ APIs, what does src/api/ contain? Your own words reveal the ambiguous nature of api/ as a directory name.

[joyeecheung]

Maybe api should be named addon-api, I think that would be clearer, and probably wouldn't even need documentation.

I don't think addon is a particularly good description though, as the C++ APIs are also for embedders - speaking of which we also have N-API headers starting with js_native_api, though I think the best way to resolve the ambiguity is probably moving them to somewhere like include.

src contains C++ APIs

If src/ contains C++ APIs, what does src/api/ contain? Your own words reveal the ambiguous nature of api/ as a directory name.

That's a typo of mine - if src contains C++ code, lib contains JS code, then for me it's not too difficult to guess that src/api contains implementations of the C++ API. At least it was pretty obvious to me that v8/src/api.cc contains code of V8's embedder API when I first started digging into the v8 source, I am not sure if that's just me though.

One could argue that having lib/internal containing internal JS code and src/api containing public C++ code creates disparity but then JS interfaces are usually public by default and C++ interfaces are private by default so I don't find that particularly hard to understand. We could make the distinction more obvious by moving the public headers into include but that's separate from this PR - even if the headers live in include it's still nice to have src/api so that we'll know we are changing public APIs when we touch src/api.

[joyeecheung]

Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/20483/

If there are no objections I'd like to land this on Friday. While I agree some of @sam-github 's concerns are valid I still think this is an improvement compared to putting all of these functions in the node.cc monolith without making it clear that they are exposed in node.h.

[joyeecheung]

Resume: https://ci.nodejs.org/job/node-test-pull-request/20491/

[joyeecheung]

Landed in 5d4b085...ca9e24e

[addaleax]

@joyeecheung This needs a manual backport for v11.x, it seems.