-
Notifications
You must be signed in to change notification settings - Fork 6
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
Should index.json be used as well as schedule.json for lts start? #9
Comments
One of the things I think we probably want both though. For example, if you do not pass a date, then I think you want the point in time and relying on the schedule lining up and doing date math is less reliable. In that case using just that field does seem like the best solution. Right? |
Interesting. That was not a use case I had considered. Strongly reproducible might be a convenience for build systems, but I am dubious it will work in a reproducible way since both schedule and index may have changed and there are not fine grained timestamps. For example:
Storing the results of the lookup seems an easy definitive approach if I want to process those results again later? My personal focus is on the right-now case and expecting current information, so I am not a customer of this feature and may be missing some compelling use cases! What is a use case for a loose definition of reproducibility and a past date? |
(Full disclosure: I am very interested in the implementation details because I am looking into how to best resolve the node support aliases in a shell script! I am not a user of |
I have been wondering whether these tests (index vs schedule) make any difference in practice, and think I have a simple example for There will be a window of time between when the "lts" version is scheduled for release, and when it becomes available. We can conveniently reproduce a check in that time window without needing to mock in test files by looking forward in time instead of back. With code that only checks the schedule, we get an incorrect result with a non-lts version returned. const nv = require('@pkgjs/nv')
async function lookup(opts) {
const versions = await nv('lts_latest', opts);
console.log(versions);
};
const opts = { now: new Date('2020-10-20T00:00:01Z') };
console.log(opts);
lookup(opts)
.then(() => console.log('done'))
.catch(err => console.log(err));
|
To take more of the information into consideration, Instead of only the date check: if (now > v.lts) should the test perhaps be:
This has a nice symmetry with using both sets of information, without introducing two modes of operation. |
@shadowspawn sorry for the long wait here. I had an idea while I was working on using this in a new library I was writing at work, and I think it might resolve my reproducibility issue better than only using the If we went the route of bundling the built index and schedule into a library it could be pinned along with other deps in the lockifiles. And then because of that, maybe we could add an option to resolve just based on the |
Optionally using a controlled (I don't fully understand the intended controlled approach, but subscribed to #10!) |
Ok, I am going to close out this issue in favor of that. I think you, @dominykas, and I agree on the goals of this and I think I can put together a decent PR which get's us there for the 1.0. I will add you as a reviewer when I open that. |
The logic in
nv
works with the dates fromschedule.json
to determine the classification of a version. I wonder ifindex.json
should be considered canon for state changes which are considered to required an actual release, in particular for whether a version is considered an LTS version. (The outcome will only be potentially different around the LTS release date.)From code inspection, I think
nv
is making the determination based on thelts
date from https://raw.githubusercontent.com/nodejs/Release/master/schedule.json at this line:nv/index.js
Line 109 in 8ce8542
However, https://nodejs.org/dist/index.json has its own field for
lts
which is either false or the release codename. The false makes it seem likely this is not intended to be treated as an lts version even if the date has passed without a new release.Rather than:
should the test perhaps be:
(The same argument is likely to apply to the start date for a version, but I have not worked through that logic yet, so raising LTS alone to check my reasoning!)
Edit: the title has changed to reflect a later suggestion to use both sources, rather than one or other as in this original comment.
The text was updated successfully, but these errors were encountered: