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

doc: update node namespace values #581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

doc: update node namespace values #581

wants to merge 1 commit into from

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Oct 26, 2023

This PR updates the node namespace values. It closes #517.

Signed-off-by: Tierney Cyren <hello@bnb.im>
Comment on lines +211 to +214
| 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). |
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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."

Copy link
Member

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.

Copy link
Member

@wesleytodd wesleytodd left a 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). |
Copy link
Member

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). |
Copy link
Member

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" |
Copy link
Member

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?

@bnb
Copy link
Contributor Author

bnb commented Oct 27, 2023

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.

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.

Copy link
Member

@wesleytodd wesleytodd left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update node namespace values in package support
4 participants