Skip to content

Conversation

@kaezone
Copy link

@kaezone kaezone commented Dec 1, 2025

This PR implements npm/rfcs issue #844, adding support for relative dates in the --before config arg (e.g. --before=24h, --before=7d, etc).

@kaezone kaezone requested a review from a team as a code owner December 1, 2025 03:17
"node_modules/ms": {
"version": "2.1.3",
"resolved": "https://registry.npmjs.org/ms/-/ms-2.1.3.tgz",
"integrity": "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we do not allow dependency changes from external PRs. This is for two reasons: first is for security, since those packages are bundled in node_modules it's best that we add those ourselves instead of try to audit the files that get added. Second, the addition of a dependency needs a discrete deps: commit so that it shows up in the changelog, and for most folks it's onerous for us to ask them to juggle commits in their PR like that.

Since this isn't introducing the dependency to node_modules itself we can verify the integrity here and allow it.

$ npm view ms@2.1.3 --json|npx json dist.integrity
sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==

If you are willing to make the addition of ms to the config workspace a discrete deps: commit, this can be considered as-is and we'll make an exception for the dependency policy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, sorry for that, wasn't 100% sure if the guideline still applied as ms is already used elsewhere in the CLI. Will split it out into a separate commit later today.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the ms change into commit 9b1a390.

@wraithgar
Copy link
Member

The current hint for this field is [--before <date>], this is customized via the hint attribute on any given definition. Is <date> still sufficient here?

@kaezone kaezone force-pushed the feature/relative-before branch from b4449e2 to 9b1a390 Compare December 1, 2025 20:15
@kaezone
Copy link
Author

kaezone commented Dec 1, 2025

Couldn't really think of a better hint attribute that was more descriptive without being too wordy - <date/relative> maybe? I think date is still fairly applicable as long as the rest of the doc string notes using relative dates.

@kaezone kaezone requested a review from wraithgar December 1, 2025 20:52
@wraithgar
Copy link
Member

I think date is still fairly applicable as long as the rest of the doc string notes using relative dates.

That seems fine to me, just wanted to bring it up in case I was missing something obvious.

@kaezone
Copy link
Author

kaezone commented Dec 1, 2025

Just curious, would this be able to fit in the 11.7.0 release (if accepted of course) or is it a bit late?

@wraithgar
Copy link
Member

The reason this hasn't been merged yet is because of hesitation with the "loosely defined human parsable date range" part of the parameter. Historically things like this feel helpful but are not. As long as folks have a consistent way to set a minimum age I think being more rigorous in what this value can be is in order.

Defining this as an integer value of hours may be sufficient. I think it's worth waiting on and considering input from others with UX knowledge.

@wraithgar
Copy link
Member

Prior art here is:

  • pnpm, which defines this as an integer of minutes
  • dependabot, which explicitly names the parameter as "days" and accepts integer values

@kaezone
Copy link
Author

kaezone commented Dec 3, 2025

Defining this as an integer value of hours may be sufficient.

Yeah I agree that'd probably be more consistent (and easier to explain in docs) - would matching pnpm with an integer of minutes be ideal?

Will wait for input before doing any more tweaking on this.

@wraithgar
Copy link
Member

Will wait for input before doing any more tweaking on this.

Best option, thanks!

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.

2 participants