-
Notifications
You must be signed in to change notification settings - Fork 145
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
doc: update node
namespace values
#581
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tierney Cyren <hello@bnb.im>
| current | | The Node.js release that is defined to be "Current" status by the [Node.js Release Process](https://github.com/nodejs/release). | | ||
| lts/latest | lts-latest | A subset of the Node.js LTS releases, only including the the _most recent_ Node.js Release that is in "Active" LTS status as defined by the [Node.js Release Process]( https://github.com/nodejs/release). | | ||
| lts/active | lts-active | A subset of the Node.js LTS releases, including _all_ of the Node.js Releases that are in "Active" LTS status as defined by the [Node.js Release Process](https://github.com/nodejs/release). | | ||
| lts/maintenance | lts-maintenance | A subset of the Node.js LTS releases, including _all_ of the Node.js Releases that are in "Maintenance" LTS status as defined by the [Node.js Release Process](https://github.com/nodejs/release). | |
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.
where does, for example, v18.18.1 fall? It's active LTS, but now v18.18.2 has precluded it. Does "lts/active" include all versions that were ever LTS in an actively maintained line? (meaning, the lower threshold only becomes a breaking change when an LTS line goes EOL)
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.
What is the status quo? My guess is that it would be contextual. For support, it would be the full range. For something like CI, I would guess it'd be most recent in-range.
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 don't think we established an official status quo here. I think that we might need lts-maintenence
and maintenance
or something similar for this use case. Or maybe do it the other way around? lts-maintenance
and lts-maintenance-all
?
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.
As a point of reference, the @pkgjs/nv
tool only returns the latest for each major when asked for lts_active
currently. This has been a point of issue for me, and so just added latestOfMajorOnly
but didn't add it to the cli flags.
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.
There is a line below the table which states: "Unless the package documentation explicitly states otherwise, the support is only implied for the latest release in any major release line."
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.
Now that you mention it, I remember us discussing this once in the original work on this. I still think we need to be much more explicit with these aliases meaning around this point.
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.
From the one discussion, I think we need specifiers for "latest of the major line" which are separate from "all versions in the group" before we move forward with this PR. The rest of my comments are more nitpicks and should not be considered blocking in any way.
| `current` | ✔ | | | | 15 | The latest release from "all" | ||
| name | alias | description | | ||
|------|-------|-------------| | ||
| current | | The Node.js release that is defined to be "Current" status by the [Node.js Release Process](https://github.com/nodejs/release). | |
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.
There is precidence in other areas for calling this latest
(the npm registry), should we align on that? We could ask the build folks to start calling it latest
?
| name | alias | description | | ||
|------|-------|-------------| | ||
| current | | The Node.js release that is defined to be "Current" status by the [Node.js Release Process](https://github.com/nodejs/release). | | ||
| lts/latest | lts-latest | A subset of the Node.js LTS releases, only including the the _most recent_ Node.js Release that is in "Active" LTS status as defined by the [Node.js Release Process]( https://github.com/nodejs/release). | |
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 implies that it can be multiple then clarifies that it is a set of one. I am not sure if this language is helpful. I think we should say it is either one release or multiple.
| `lts` | `lts/active, lts/maintenance` | The package is maintained for the Node.js LTS releases (both in Active and Maintenance status). Anyone creating an application using an LTS version of Node.js and using the latest major version of LTS adopting packages will not have to accept semver-major level (ie. breaking) changes into that application in order to receive essential fixes. Full details are available [here](https://github.com/CloudNativeJS/ModuleLTS) | | ||
| `lts_latest` | `lts/latest` | The package is maintained only for the Latest LTS Node.js version. You will be required to update to the latest LTS Node.js version in order to ensure you can use new versions/get security fixes | | ||
| `supported` | `current, lts/active, lts/maintenance` | Node.js versions which are not EOL | | ||
| `current` | `current` | The latest release from "all" | |
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.
FWIW, I think the old vs translated here is confusing in some examples. I think partly that is because the new ones cover more nuanced meaning and for example all
as the old one is not even present in the new context. I honestly would rather just drop the "old description" and replace that with "whats changed" or something?
This point is a distinct issue with how this was initially implemented, not with this PR. This PR doesn't change the status quo - that should be done in a different PR, as it's a different set of work. I'd be happy to work on that separately, but this PR should IMO be judged on how it stacks against the status quo and the gaps in that should not be the responsibility of the PR to fix. |
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.
Hm, I think I disagree because even though this is not a code repo, this is a breaking change and so would be the change to address this gap. Since "publishing" here is just merging this, I would prefer if we landed all the related breaking changes in one go. I guess I am good with doing it in two PR's where we block both until they are both ready.
That said, I don't think this is important enough to block progress and I am fully in support of these changes, so I will withdraw my blocking review/comment.
This PR updates the
node
namespace values. It closes #517.