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

To be or not to be in core #1041

Closed
ronag opened this issue Jun 18, 2021 · 42 comments
Closed

To be or not to be in core #1041

ronag opened this issue Jun 18, 2021 · 42 comments

Comments

@ronag
Copy link
Member

ronag commented Jun 18, 2021

The discussion regarding whether or not s module should be in core seems to pop every now and then and I feel it's a little unfortunate that it kind of always has to start over from almost nothing and with different people involved.

Whether something should be or shouldn't be in core is quite complicated and it will be difficult to agree on any consistent set of rules for this that an be applied across the board. However, I think we should still be able to write up something in terms of what things we consider, what is advantages and disadvantaged to either approach etc. So that when these discussions pop up we have some form of summarized and organized prior knowledge/discussion/thoughts document to refer to.

I think it would maybe be appropriate for the TSC to provide some form of guideline here.

Some examples where it's unclear whether things should be in core or not:

and probably more...

As I see it we have 3 possible approaches on a module by module basis:

  1. include in core
  2. include in some kind of "standard library" (e.g. npm @node/package-name).
  3. external package which lives under the nodejs org in GitHub (e.g. undici)

Some initial notes from me to start from:

  • Part of LTS workflow
  • CITGM
  • More "trust"
  • Other core modules can use the functionality
  • Same as 3 but maybe with more "trust"
  • Higher development velocity.
  • Higher publishing velocity. User can access fixes and new features quicker.
  • More fine grained step by step upgrade by users in terms of breaking changes.
@targos
Copy link
Member

targos commented Jun 18, 2021

  • Ability to create standalone scripts that take advantage of the feature without the need of npm/package.json/dependencies

@ronag
Copy link
Member Author

ronag commented Jun 18, 2021

I think there is also:

  1. develop inside core and publish to npm (readable-streams)
  2. Develop outside of core and sync it in every now and then (llhttp)

@Trott
Copy link
Member

Trott commented Jun 19, 2021

@nodejs/tsc

@ronag
Copy link
Member Author

ronag commented Jun 19, 2021

The undici discussion got a little heated nodejs/node#39057 which is also a reason it would be nice to be able to provide a bit more context I. Those discussions.

@ronag
Copy link
Member Author

ronag commented Jun 19, 2021

@jasnell also had a nice write up on Twitter https://twitter.com/jasnell/status/1405682071440297986?s=21

@mhdawson
Copy link
Member

For

Simplified build/better cross platform testing if native code is required
Good performance can sometimes only be achieved through integration in core.

@tniessen
Copy link
Member

Also:

  1. include in core
  • consistent API (e.g., http ~ https ~ http2)
  1. external package which lives under the nodejs org in GitHub (e.g. undici)
  • allows APIs that aren't consistent with Node.js core conventions

@Trott
Copy link
Member

Trott commented Jun 23, 2021

This was discussed briefly in TSC. Worth documenting. Needs more input from other @nodejs/tsc folks. Please review and comment!

@bmeck
Copy link
Member

bmeck commented Jun 23, 2021

I wasn't there, but I would add that various efforts to be robust (mainly coding styles and primordials) are not really able to be enforced easily. To do so you have to do things like I did with MIME being able to be in a different repo : https://github.com/bmeck/node-internals-mime . I think having consistent patterns to avoid various concerns of cross module workflows gets complicated if we fracture into many different repositories but don't have any guidelines for those repositories.

@mmarchini
Copy link
Contributor

During today's TSC meeting it was suggested that we have a separate meeting with folks interested in this issue. Would folks be interested in that, and if so does anyone volunteer to coordinate the meeting?

@bmeck
Copy link
Member

bmeck commented Jul 1, 2021

I would like to be in the meeting, I can try and coordinate but would have to put efforts on things on my backlog.

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

One approach we discussed during the TSC meeting would be vendoring in modules, similar to llhttp and npm. Given the undici example; undici is developed outside of core but we vendor in specific releases (/deps) that are deemed stable. That way we maintain high development velocity outside of core whilst maintaining the LTS/CITGM workflow for core. This approach seems to work well with npm and llhttp so there is some precedence and I think this can to a large part also apply to other modules mentioned above.

The open question here is how to handle the documentation. I wonder if we can easily vendor in the documentation as well given that the module uses the same doc style? That shouldn't be too much work.

This also makes it easier to actually get into something into core as e.g. the whole undici test suite doesn't need to be re-written to align with how node does testing (I have a hard time seeing who would be willing to spend time doing that).

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

Pinging some people that I believe might have thoughts on the matter: @jdalton @addaleax @MylesBorins @devsnek @mscdex @lpinca @jasnell

@Flarna
Copy link
Member

Flarna commented Jul 2, 2021

One difference to npm/llhttp is that undici exposes a public API in node. As a result any breaking change done in undici can't be hidden in a glue layer implementation like it is possible for llhttp and other internal deps.

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

As a result any breaking change done in undici can't be hidden in a glue layer implementation like it is possible for llhttp and other internal deps.

I'm not sure that's true for npm though? But yes, any semver major change in undici could only be vendored in during a semver major node core release. I don't see any problem with that.

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

Another question. If we vendor in js modules. Should we have a rule that they must live under the node org in Github? So that we have control?

@Flarna
Copy link
Member

Flarna commented Jul 2, 2021

But yes, any semver major change in undici could only be vendored in during a semver major node core release. I don't see any problem with that.

It's no problem. It just means that these vendored modules need at least some sort of maintainance strategy matching to core for e.g. backporting, testing and releasing fixes,....
Clearly this would be the same if they are integrated in node but the infrastructure for release branches, CI is already in place there.

Or in ohter words, I think some burden of "being part of the node binary" needs to be acepted by these modules. Otherwise node may end up in exposing an API of an unmaintained package.

I guess one requirement is also that the license of these modules allows that node core may decide to fork it and maintain their own variant.

Clearly these are just worst case scenarios, I assume it just works fine on default.

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

I guess one requirement is also that the license of these modules allows that node core may decide to fork it and maintain their own variant.

Yes, that goes a bit under my other question on whether we should require these modules to live under the node org, e.g. we've already moved undici to node/undici.

@aduh95
Copy link
Contributor

aduh95 commented Jul 2, 2021

FWIW currently we vendor https://github.com/guybedford/cjs-module-lexer, which is outside of nodejs org, introduced in nodejs/node#35249. It seems to meet all the requirements suggested in #1041 (comment), so it seems to be sufficient IMHO.

@ronag
Copy link
Member Author

ronag commented Jul 2, 2021

Another question. If we vendor in js modules. Should we have a rule that they must live under the node org in Github? So that we have control?

This would also help a little with filtering suggestions, i.e. if the author is not willing to move it into the node org then maybe it shouldn't be in core?

@bmeck
Copy link
Member

bmeck commented Jul 2, 2021

@aduh95 yes, we actually also vendor in acorn which is also outside of nodejs. These modules however do not follow the defensive coding patterns of node core. In part because we can't easily ship things to npm that do follow those patterns. Hence my wanting some kind of enforcement of quality and robustness since we are adopting the potential attack vectors etc. of those modules. Having different coding standards inside of our runtime and the vendored dependencies doesn't make sense to me. We go through tremendous effort to do things like primordials which are a big part of landing a PR in core these days. Having lax security audits of vendored deps is somewhat historically accurate but if we fan out to have many more vendored deps we need a plan on how to deal with this consistently.

@ronag
Copy link
Member Author

ronag commented Jul 3, 2021

@aduh95 yes, we actually also vendor in acorn which is also outside of nodejs. These modules however do not follow the defensive coding patterns of node core. In part because we can't easily ship things to npm that do follow those patterns. Hence my wanting some kind of enforcement of quality and robustness since we are adopting the potential attack vectors etc. of those modules.

Maybe a good reason to have them under the node org?

Having different coding standards inside of our runtime and the vendored dependencies doesn't make sense to me.

It's not optimal but I'm not sure how to get around that?

We go through tremendous effort to do things like primordials which are a big part of landing a PR in core these days.

Primordials are being discussed. This doesn't feel like it deserves to be a blocker.

Having lax security audits of vendored deps is somewhat historically accurate but if we fan out to have many more vendored deps we need a plan on how to deal with this consistently.

How would that differ in terms of vendoring?

@danielleadams
Copy link
Contributor

Another question. If we vendor in js modules. Should we have a rule that they must live under the node org in Github? So that we have control?

I think this is a good start to make sure there's more control, but we'll want to have processes and automation set up (and get the maintainers onboard) to ensure that each project joins the release cycle workflow.

I'm generally in favor of adding modules to Node. I'd be curious what the implications of security patches would be though. It's easy with just npm - we work closely with them and can be flexible because Node releases are accommodating 1 external project. I'm hesitant if that number becomes 5, or 10, how much that would slow down the release cycle if there are more frequent patches going out. If the governance becomes internal, maybe not at all.

I wonder if we can easily vendor in the documentation as well given that the module uses the same doc style? That shouldn't be too much work.

If the modules don't all use the same doc style, then we'll need to standardize. Will Node.js expand the Node.js API docs or modules stay in their own docs?

@bmeck
Copy link
Member

bmeck commented Jul 6, 2021

Maybe a good reason to have them under the node org?

I'm not clear on what moving them into the org would change much? Are we committing to changing their codebases to match various coding patterns we expect / maintaining the modules rather than their existing maintainers?

It's not optimal but I'm not sure how to get around that?

Linting/CI seems like it works inside of Node core already for our builtins, we can enforce our needs via tooling similarly in other repositories.

Primordials are being discussed. This doesn't feel like it deserves to be a blocker.

The migration in core is non-trivial in vendored code since primordials are not intended to be used by userland code. That is why it is somewhat a blocking concern. If core continues to feel the need to be robust using primordials vendoring an increasing amount of code that does not comply with that concern for robustness. If we don't care about robustness that means this concern doesn't need to be addressed by vendored modules but we haven't agreed upon that currently.

How would that differ in terms of vendoring?

The amount of things vendored into core is minimal and hasn't changed largely over time. Adding lots of vendored modules would mean an increasing surface area of vendored dependencies that we haven't been seeing in core with our existing vendored dependencies that are relatively stable.

@bmeck
Copy link
Member

bmeck commented Aug 12, 2021

I'm a bit late due to things, but if I could get link as well that would be swell

@DerekNonGeneric
Copy link

I'm a bit late due to things, but if I could get link as well that would be swell

Sorry we missed you @bmeck, we briefly touched on having that MIME module in core.

Pretty sure that we streamed the whole meeting, but for some reason can't seem to find the video on YouTube. (?)

@targos
Copy link
Member

targos commented Aug 12, 2021

Video is here: https://www.youtube.com/watch?v=7Rqe4pT5XtM

@ronag
Copy link
Member Author

ronag commented Aug 12, 2021

I'm a bit late due to things, but if I could get link as well that would be swell

I did send you the link while in the meeting. You didn't get it?

@GrosSacASac
Copy link

Not yet mentioned but I think it is important: Funding,
If a project is included into core maybe it has more chances to get funding from individuals and the open js foundation.

@danielleadams
Copy link
Contributor

I've started a discussion here: nodejs/node#39779

@ronag
Copy link
Member Author

ronag commented Aug 25, 2021

I've missed this comment. Will fill it up as soon as I get them.

Trott added a commit to Trott/io.js that referenced this issue Oct 5, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041
Trott added a commit to Trott/io.js that referenced this issue Oct 6, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041
Trott added a commit to Trott/io.js that referenced this issue Oct 15, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041
Trott added a commit to Trott/io.js that referenced this issue Oct 20, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041
@Trott
Copy link
Member

Trott commented Oct 22, 2021

89c0577 closed this because the original ask was to document considerations. But if there's more that needs to happen (and I suspect there is), let's either re-open this or create a new issue. /cc @ronag

targos pushed a commit to nodejs/node that referenced this issue Oct 23, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041

PR-URL: #40338
Fixes: nodejs/TSC#1041
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this issue Nov 24, 2021
Document the things that are considered when making the determination as
to whether something should or shouldn't be in core. This does not (yet,
at least) attempt to address *how* to include modules in core. (Should
it be in the Node.js code base or vendored in from a separate
repository?) It is limited to *whether* something should be in core or
not.

Closes: nodejs/TSC#1041

PR-URL: #40338
Fixes: nodejs/TSC#1041
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.