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

feat: add option to ignore future releases #39

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

Conversation

jenseng
Copy link

@jenseng jenseng commented Nov 12, 2024

Add an ignoreFutureReleases flag to ignore releases with dates after now. This is useful when specifying a now in the past (e.g. to ignore an in-flight release, as is currently the case with v18.20.5)

Alternatively we could just update the now logic to always do this, but that would be a breaking change and half the tests would need to be reworked 😆 😭

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.

I almost think the now flag should work like this by default, but that would be a major. Maybe we can swap this to be opposite in a future major?

@ljharb
Copy link
Member

ljharb commented Nov 13, 2024

tbf the situation with 18.20.5 feels very very uncommon - there's usually not much of a delay at all between updating the tab file and uploading the artifacts.

i'd think a better approach than an option like this is to filter out versions that don't have files present on the server, in nv itself.

@jenseng
Copy link
Author

jenseng commented Nov 13, 2024

Does nodejs.org currently provide an endpoint that could tell us that with a single request (e.g. extended version of dist/index.json)? Otherwise it could be a tad inefficient to determine this, depending on how many aliases/versions we're evaluating

@ljharb
Copy link
Member

ljharb commented Nov 13, 2024

You'd only need to check versions that were released within the past 24 hours, that were also the latest version of the release line.

@jenseng
Copy link
Author

jenseng commented Nov 13, 2024

That's fair, that wouldn't be so bad.

Really though doing anything within nv around this feels like a workaround for a bug in node's release process. i.e. as a user of node, I would expect that when a version appears in dist/index.json (or whatever), I should immediately be able to download it, without having to do extra calls, similar to https://registry.npmjs.org/<somepackage>. I'm sure that's easier said than done though ❤️

@ljharb
Copy link
Member

ljharb commented Nov 13, 2024

Indeed - tbh I'm not sure it's worth working around it with anything, including this PR :-)

@jenseng
Copy link
Author

jenseng commented Nov 13, 2024

closing, as the v18.20.5 dist issue is resolved 😅

if this problem resurfaces again, a workaround could be to bypass cloudflare altogether and download binaries from https://direct.nodejs.org/dist/ (assuming that's supported)

@jenseng jenseng closed this Nov 13, 2024
@jenseng jenseng deleted the ignore-future-releases branch November 13, 2024 01:18
@wesleytodd
Copy link
Member

wesleytodd commented Nov 13, 2024

I dislike closing this. I understand the feedback and saying it is not really a problem from the perspective of today's issue, but my point stands that I think this behavior is probably more what users might expect with the --now flag. @ljharb I didn't see you address that above, do you disagree on that part?

feels like a workaround for a bug in node's release process

I am following up with this here: https://openjs-foundation.slack.com/archives/CK9Q4MB53/p1731450996448959

EDIT: I didn't re-open although I could, but I wanted to just say that so @jenseng could decide (as it is your PR), but that is personally what I would prefer we do so it does not appear "decided" when we only just had a convo today and not many other folks have had a chance to comment. Typically I would only close something this quickly if it was CLEARLY invalid or never going to happen, which I don't think we achieved consensus on in the above convo.

@jenseng jenseng restored the ignore-future-releases branch November 13, 2024 03:15
@jenseng jenseng reopened this Nov 13, 2024
@jenseng
Copy link
Author

jenseng commented Nov 13, 2024

Sure, although my immediate need is resolved, I'm happy to leave this open for further feedback/discussion.

I also agree that in the long term the now flag should just have this behavior, as it makes the most sense (though that would be a breaking change if anyone besides me is using now 😆)

@ljharb
Copy link
Member

ljharb commented Nov 13, 2024

i probably didn't understand what the PR does.

certainly if an explicit "current time" timestamp is passed, releases from after it should simply not appear to exist, just like npm's --before flag.

@wesleytodd
Copy link
Member

Yep, I think my initial use for this was more for testing and I coupled it with input which worked with that design. But in hind sight I think this new approach is a much better api, especially for end users (not just testing).

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.

3 participants