Skip to content
This repository has been archived by the owner on Apr 18, 2018. It is now read-only.

doc: first pass at stability policy #21

Merged
merged 3 commits into from
Apr 7, 2015
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,80 @@ By making a contribution to this project, I certify that:

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

## Stability Policy

The most important consideration in every code change is the impact it will have, positive or negative, on the ecosystem (modules and applications). To this end, all Collaborators must work collectively to ensure that changes do not needlessly break backwards compatibility, introduce performance or functional regressions, or negatively impact the usability of the Project on any of the platforms officially targeted for support by the Project. The TSC is responsible for determining the list of supported platforms.
Copy link

Choose a reason for hiding this comment

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

This is great wording, much better than what I wrote in our Stability Policy :)


When discussing stability, it is important to first review Node.js' layered architecture:

```
+---------------------------------------------------+
| Module and Application Ecosystem |
+---------------------------------------------------+
| | | | |
| | +----------------------+ |
| | | Native Abstractions | |
| | | for Node.js | |
| | +----------------------+ |
| | | | |
+----------------+ | | | /
| Node.js Core | | | | /
| Library API | | | | /
+----------------+ | | | /
| js impl | | | | /
+----------------+ | | | /
| | | | /
+--------------------------------------+ |/
| Node.js Application Binary Interface | |
+--------------------------------------| |
| C/C++ impl | |
+--------------------------------------+ |
| |
+---------------------------------------+
| Dependencies: v8, libuv, openssl, etc |
+---------------------------------------+
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, but, should only be one line above NAN :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nan should be used twice as much? Lol... Thanks will fix
On Apr 7, 2015 2:06 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In README.md
#21 (comment):

  • | Node.js Core | | | | /
  • | Library API | | | | /
  • +----------------+ | | | /
  • | js impl | | | | /
  • +----------------+ | | | /
  •        |            |     |             | /
    
  • +--------------------------------------+ |/
  • | Node.js Application Binary Interface | |
  • +--------------------------------------| |
  • | C/C++ impl | |
  • +--------------------------------------+ |
  •                  |                      |
    
  •    +---------------------------------------+
    
  •    | Dependencies: v8, libuv, openssl, etc |
    
  •    +---------------------------------------+
    
    +```

Just a nit, but, should only be one line above NAN :)


Reply to this email directly or view it on GitHub
https://github.com/jasnell/dev-policy/pull/21/files#r27920435.

Copy link
Contributor

Choose a reason for hiding this comment

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

(It should be used twice as much as direct C++ APIs though. :P)


Node.js currently builds on top of several key dependencies including the V8 Javascript engine, libuv, openssl and others. The Node.js Application Binary Interface provides critical functionality such as the Event Loop which critical to how Node.js operates. The Node.js Core Library is the primary interface through which most Modules and Applications built on top of Node perform I/O operations, manipulate data, access the network, etc. Some modules and applications, however, go beyond the Core Library and bind directly to the Application Binary Interface and dependencies to perform more advanced operations. The Native Abstractions for Node.js (`nan`) is binary abstraction layer used to buffer module and application developers from changes in the Application Binary Interface and Dependencies.
Copy link

Choose a reason for hiding this comment

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

s/Javascript/JavaScript


Due to the existing layering, modules and applications are sensitive not only to changes in the Core Library API, but the Application Binary Interface and dependendencies as well.

Any API *addition* to either the Node.js Core Library API or Application Binary Interface must result in a *semver-minor* version increase.
Copy link

Choose a reason for hiding this comment

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

So, the current io.js Stability Policy states that changes to the ABI that can be handled by nan are minor and ones that are not are major.

However, in practice this is not what we've done and have been more conservative. Towards the end of v8's last cycle they took a very minor change that altered the ABI. We kept that change out, it was quite trivial and we all agreed wasn't worth forcing everyone to suffer an addon recompile.

I remember that in the TC discussion about that change many TC members felt that if we're going to force an addon recompile it should only be on a major version bump. This is worth discussing in the next meeting as well, I think there are some differences of opinion here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps something like "Any modification to the Application Binary Interface or Dependencies that would require module or application developers to recompile must result in a semver-major version increase."

Copy link

Choose a reason for hiding this comment

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

@jasnell that might be good but I would hold off on making the change until we discuss it on Thursday. Let's put this down as an agenda item :)


Any *backwards incompatible* change to either the Node.js Core Library API or Application Binary Interface must result in a *semver-major* version increase.

Issue: Should any modification to the ABI or Dependencies that requires module or application developers to recompile force a *semver-major* change?
Copy link
Contributor

Choose a reason for hiding this comment

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

Policy for this in io.js is semver-minor.

Copy link

Choose a reason for hiding this comment

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

@Fishrock123 that's the policy but in practice we've held off on changes in a minor that would do this and that was driven by the TC so we may want to revisit it.

Copy link

Choose a reason for hiding this comment

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

I think if we could get over our psychological hurdle against minting new version numbers then semver-major makes the most sense. But right now we're hesitant to release a 2.0.0 and thus missing ABI changes (notably a floating patch on top of V8!) that need to be made.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I think we'll get there soon tho


APIs and default behaviors in the Node.js Core Library, Application Binary Interface, Dependencies and nan must not change within LTS Releases unless the change is required to address a critical security update.

Note that default or assumed behaviors and values are exposed via the API, ABI or Dependencies are considered to be part of the API. A change in a default value, for instance, counts as an API change as modules and applications may be written to assume certain defaults and could be broken.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure this is always the case.

I.e. Certain libuv things, etc.

Copy link

Choose a reason for hiding this comment

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

Ya, this makes me wonder what constitutes a "default value." For instance, error message text defaults were change in libuv, that wasn't a major bump.

Choose a reason for hiding this comment

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

This also puts changes like this one in a weird position – technically this breaks existing behavior and should be semver-major, but we're nearly 99% sure that no one is relying on the ability to redefine constants, so it's in at patch level. On the other hand, we were nearly 99% sure that making require('.') work as expected would affect approximately no one, so it was merged in at semver-patch (indeed, the only level of change allowed by the "locked" status of the module system). However, it resulted in a fairly arcane workflow breaking. Is that change semver major, or patch?

Copy link

Choose a reason for hiding this comment

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

However, it resulted in a fairly arcane workflow breaking. Is that change semver major, or patch?

IMO it should have been major and probably would have been if we knew ahead of time that it would break this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps: "Default or assumed behaviors that, if modified, could cause modules or applications to break are considered part of the API." This would exclude things like error messages which are generally informational but would catch such things as, say, openssl default cipher suites, etc.

Copy link

Choose a reason for hiding this comment

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

I think it's more practical to use the browser-esque standard of "this is technically backward incompatible but in practice should not be." E.g. transitioning from throwing an error to not throwing an error could be backward incompatible for code that depends on the error being thrown, or changing a message string could be backward incompatible for code that parses the string, or removing a underscore-prefixed API could be backward-incompatible for code that abuses it. But in reality those cases should be rare and are allowed to break. @petkaantonov sums this up well in nodejs/node#1356 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point @domenic. I can continue to tweak the wording on this particular bit


### Dependency Stability

#### JavaScript Support (V8)

Node.js will adopt new V8 releases as quickly as practically feasible. For LTS Releases, the version of V8 shipped must be supported and fully operational on all platforms officially targeted for support.
Copy link

Choose a reason for hiding this comment

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

We may want to explicitly state that the ABI cannot change on LTS releases unless it is required for a critical security update. LTS consumers will definitely not be expected to have to recompile their addons.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about just reverting ABI changes to LTS releases?

Copy link

Choose a reason for hiding this comment

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

"the version of V8 shipped must be supported and fully operational on all platforms officially targeted for support" unless you are shipping new LTS (and removing old ones) every six weeks, I don't see how this is possible, since V8 versions older than the current one are not supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Supported" is perhaps the wrong word. When a new LTS is cut, the version of V8 shipped should build / be functional / not introduce any regressions for any of the officially supported platforms. It's really a stability requirement. If we ship an LTS with a V8 that works on Platforms A, B, and C one week, we shouldn't ship an LTS with a V8 that only works on Platforms A and B a few weeks later unless Platform C is removed from our officially supported platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, we may want to refine some of the language around "Officially supported platforms"

Copy link

Choose a reason for hiding this comment

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

Yes, please remove the word "supported" then, in reference to old V8 versions.


When V8 ships a breaking change to their C++ API that can be handled by `nan` there will be a *semver-minor* version increase.

When V8 ships a breaking change to their C++ API that cannot be handled by `nan` there will be a *semver-major* version increase.

When new features in the JavaScript language are introduced by V8, there will be a *semver-minor* version increase. TC39 has stated clearly that no backwards incompatible changes will be made to the language so it is appropriate to increase the *minor* rather than the *major*.

Pull Requests that introduce post-ES5 mechanisms into the Node.js Core Library API require TSC review and approval. If landed, such changes must result in a *semver-major* version increase. However, Pull Requests may introduce post-ES5 mechanisms into the *internal* JavaScript implementation of the Core Library API so long as those mechanisms do not "bleed out" through the API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this harmful? Why would these be semver-major? It's not like these would need to be flagged or use external libs...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for things like adding promises into the core API. It's been a
significant enough of an issue historically that such changes warrant
additional review. If there is consensus on the TSC that this isn't a
concern, then this can be dropped but I feel it at least warrants
discussion before doing so.
On Apr 7, 2015 2:15 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In README.md
#21 (comment):

+Note that default or assumed behaviors and values are exposed via the API, ABI or Dependencies are considered to be part of the API. A change in a default value, for instance, counts as an API change as modules and applications may be written to assume certain defaults and could be broken.
+
+### Dependency Stability
+
+#### JavaScript Support (V8)
+
+Node.js will adopt new V8 releases as quickly as practically feasible. For LTS Releases, the version of V8 shipped must be supported and fully operational on all platforms officially targeted for support.
+
+When V8 ships a breaking change to their C++ API that can be handled by nan there will be a semver-minor version increase.
+
+When V8 ships a breaking change to their C++ API that cannot be handled by nan there will be a semver-major version increase.
+
+When new features in the JavaScript language are introduced by V8, there will be a semver-minor version increase. TC39 has stated clearly that no backwards incompatible changes will be made to the language so it is appropriate to increase the minor rather than the major.
+
+Pull Requests that introduce post-ES5 mechanisms into the Node.js Core Library API require TSC review and approval. If landed, such changes must result in a semver-major version increase. However, Pull Requests may introduce post-ES5 mechanisms into the internal JavaScript implementation of the Core Library API so long as those mechanisms do not "bleed out" through the API.

Isn't this harmful? Why would these be semver-major? It's not like these
would need to be flagged or use external libs...


Reply to this email directly or view it on GitHub
https://github.com/jasnell/dev-policy/pull/21/files#r27921267.

Copy link

Choose a reason for hiding this comment

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

We haven't done this yet, and have no plans to, so I don't quite see why we are already creating a policy around it. You could characterize the existing policy as "we aren't adding ES5 features to the core API" and that will remain true until we do, at which point we'll be far more informed and can write a better policy.

I'm also concerned with how widely defined "post-ES5 mechanism" could be. For instance, the unhandledRejection event was added in a minor release. This feature related to post-ES5 features, namely promises, but does not use promises itself or require the use of ES5 features to access it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra review is fine for now, but that shouldn't force a non-breaking change to be semver-major.

Also, the majority of likely additions in this area are not as radical as Promises.
Symbols, Sets, and Maps come to mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could characterize the existing policy as "we aren't adding ES5 features to the core API"

I'm not actually sure that is the current state of things. There have been a lot of places where Symbols have been recommended by TC members recently.

Copy link

Choose a reason for hiding this comment

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

+1 semver-minor not semver-major.

Copy link
Member Author

Choose a reason for hiding this comment

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

@domenic I've iterated on this language. Can you check the current version and see if it works for you?

Copy link

Choose a reason for hiding this comment

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

No, it does not work for me. Adding a feature should not bump the major version if that feature is added in a backward compatible way. Gating on language versions (which are fictional constructs anyway) doesn't change this.

Copy link

Choose a reason for hiding this comment

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

Oh, I'm sorry, I didn't realize the iteration was in a follow-up PR. It does look better there.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, the edits are coming rather quickly as I'm trying to get a stable draft in by today to give everyone time to review before tomorrows call. I'm trying to keep at most one active PR at a time so if you look at the master and see an open PR, the most recent edits are in the PR. If there's no open PR, then the master is the latest. I definitely appreciate you jumping in with comments. It's very helpful


#### Other dependencies

Other dependencies such as OpenSSL, libuv and http-parser are handled similarly.

Node.js will continue to adopt new dependency releases as often as practically feasible.

When a dependency ships a breaking change to their API, if that change can be abstracted by the Node.js Application Binary Interface such that module and application developers can be isolated from the change without breaking existing applications, there will be a *semver-minor* version increase.

When a dependency ships a breaking change to their API that cannot be abstracted by the Node.js Application Binary Interface, there will be a *semver-major* version increase.

When dependencies introduce additional functionality that does not break backwards compatibility in any way, there will be a *semver-minor* version increase.

## A Few Open Questions

1. What about things like http_parser and libuv? Does it make sense to see about bringing each into the foundation as their own projects?
Expand Down