-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(config): parse relative date in --before #8802
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
base: latest
Are you sure you want to change the base?
Conversation
| "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==", |
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.
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.
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.
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.
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.
Split the ms change into commit 9b1a390.
|
The current hint for this field is |
b4449e2 to
9b1a390
Compare
|
Couldn't really think of a better hint attribute that was more descriptive without being too wordy - |
That seems fine to me, just wanted to bring it up in case I was missing something obvious. |
|
Just curious, would this be able to fit in the 11.7.0 release (if accepted of course) or is it a bit late? |
|
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. |
|
Prior art here is:
|
Yeah I agree that'd probably be more consistent (and easier to explain in docs) - would matching Will wait for input before doing any more tweaking on this. |
Best option, thanks! |
|
As other users mentioned in the alternative PR #8825, this approach of extending Maybe it makes sense to reconsider this alternative PR #8825, as it follows the standard patterns introduced by pnpm |
|
FWIW I'm not particularly attached to my PR, we're just looking for any variation of this feature at my workplace. |
|
Thanks @kaezone it is still a good PR and we're glad you made it. This is a topic that's obviously top of mind for a lot of folks and this PR has become a great focal point for the discussion. |
Yuuuup, I made a Github account only for joining this discussion cuz was interesting, I was searching for if npm has a minimum age release rule or something and found this. I comply with @karlhorky . @wraithgar you should reconsider re-opening #8825 PR. Implements a minimum release age policy very similar as pnpm's and Bun's. Not pretending to ofend, this PR is really good, but I don't think that is what the community wants to see on NPM. After all, the final usser/developer is the one who wants to code without caring about vulnerabilities in dependencies/sub-dependencies and set a minimum required age for the releases of the packages and with that, they can be "safer" in some way. As many people say in #8825, the feature implemented at this PR, has no form of excluding packages from this policy. In my point of view as a developer, I would prefer having the possibility of excluding some packages.
I believe both implementations should be implemented, but for a more complete implementation, in my opinion and that of several users and even contributors, the best option currently would be to implement PR #8825. However, I also don't want this PR to be closed, as it can also be improved beforehand, allowing both implementations to work harmoniously together. |
|
I'm generally in agreement here as well - the other PR looks quite a bit more robust. This PR was intended as the minimum necessary to extend the current functionality to help reduce a risk I'm working on (and presumably one many other dev shops face), but the other one provides quite a bit more in terms of QOL. For what it's worth, I primarily work in the .NET appsec and GRC spaces, so I'd tend to lean towards the opinion of those who spend more time in the JS/TS ecosystem :) |
This PR implements npm/rfcs issue #844, adding support for relative dates in the
--beforeconfig arg (e.g.--before=24h,--before=7d, etc).