-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
A *first pass* at the stability policy. This is based in part on the io.js roadmap (https://github.com/iojs/io.js/blob/v1.x/ROADMAP.md) but is expanded and clarified.
@@ -183,6 +183,69 @@ 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. |
There was a problem hiding this comment.
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 :)
doc: first pass at stability policy
+---------------------------------------+ | ||
| Dependencies: v8, libuv, openssl, etc | | ||
+---------------------------------------+ | ||
``` |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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" [email protected]
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.
There was a problem hiding this comment.
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)
Note: NAN should be capitalized. |
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Also, this doesn't cover npm. npm is quite different than other deps; it's not actually part of our API. Also, it is often bumped independently, and users may very likely not even use the bundled npm. (There is also a lot of talk about having options to not install the bundled npm, but that currently isn't possible on io.js) As such, the current (but not actually official) io.js policy is that npm is simply semver-patch, virtually always. Discussion here: nodejs/node#942 |
I don't think that is entirely accurate. We've been taking semver-minor npm bumps in patch releases but npm 3 is actually a big user facing API change and even a change to the way packages are installed in the tree, if there was a PR to merge it I think we'd force a major. |
+---------------------------------------+ | ||
``` | ||
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Javascript/JavaScript
A first pass at the stability policy. This is based in part on the
io.js roadmap (https://github.com/iojs/io.js/blob/v1.x/ROADMAP.md)
but is expanded and clarified.