Repository navigation
A package-lock.json for eslint? #142902
Description
Recently there was a proposal to run npx eslint without first npm installing it.
However, I personally have no idea how dependencies would have been resolved for doing this. Normally, for code in the repository, we have a Cargo.lock that decides what will be resolved and we deliberately bump it when we want to update. This provides some assurance, at least, that we don't resolve random packages.
Part of my confusion with that PR was... that. If we're going to run JS testing and linting, then it's unclear to me why we don't also have locks so that we know what code we are running.
Activity
jieyouxu commentedon Jun 23, 2025
I believe this was answered by @yotamofek in a review comment chain here
yotamofek commentedon Jun 23, 2025
I think it's a partial answer to a slightly different question.
That's what
npxdoes - it runs packages without having to explicitly install them first. That PR you are referring to (#142851), in its original version, wouldn't have changed anything re: version resolution etc.That's a good point.
npxornpm installaside, even if we pick specificeslintversion (and we do since the implicit semver operator in thenpmworld is=, as opposed to cargo which implicitly turns something like0.x.yinto>=0.x.y, <0.x+1), there are two additional guarantees apackage-lock.jsonwould give us:eslintversion, which can guard against certain cases of MitM, I guess?eslint's recursive dependencies - even ifnpxornpm installchooses a specificeslintversion, that doesn't mean some deeply nested dependency can't resolve to a newer version, in accordance with the semver range specified for it, which opens us up to the good ol' supply-chain attackSo yeah, I think a
package-lock.json(plus--freeze) for anything run bytidywould not hurt.Noratrieb commentedon Jun 23, 2025
Yeah, we should have a package-lock to lock the dependencies of eslint. I've had many bad experiences before at work work eslint and things breaking :).
yotamofek commentedon Jun 23, 2025
Cool, I can do that (along with making
eslintrun only if changes were detected in JS files)@rustbot claim
2 remaining items
GuillaumeGomez commentedon Jun 27, 2025
Important to be noted: since we install JS dependencies in docker builds, the
package-lock.jsonfile would need to live insrc/ci/docker, which is pretty bad. Hence why currently, we track version in a file in this folder which is retrieved bybootstrapand then check that's the current version installed.I don't think putting the
package-lock.json(orpackage.json) file there would make sense, hence why I didn't do it. I'd love to make things simpler for everyone but currently it seems to not be possible or to be very ugly.Kobzol commentedon Jun 27, 2025
To clarify, we can keep the lockfile outside of
src/ci/docker, as long as we install the JS packages at (Docker image) runtime, rather than at (Docker image) build time.workingjubilee commentedon Jun 28, 2025
I do love beautiful solutions.
But I think "ugly but works", when the matter is at most an extra few files of no significant size, is better than "reinventing the incredible complexities of dependency management in an ad hoc manner, strewn across completely unrelated parts of the codebase".
GuillaumeGomez commentedon Jun 28, 2025
@lolbinarycat I reassigned @yotamofek as they are still working on it as far I know and even have an open PR.
GuillaumeGomez commentedon Jun 28, 2025
Agreed, hence why I suggested to only run JS checks if a JS file was modified.
yotamofek commentedon Jun 28, 2025
I was gonna work on it after @lolbinarycat finished #142924 , but she hit me up on Zulip and said she was gonna work on this and integrate it into her PR, so I told her to assign this issue to herself. :)
Haven't started working on this yet, and definitely don't have a PR, so had no problem with her taking over.
GuillaumeGomez commentedon Jun 28, 2025
Ah my bad, then reassigning @lolbinarycat (please write a message so people not involved in conversation understand what's going on 😆 ).
lolbinarycat commentedon Jun 29, 2025
Yeah I don't think it makes sense to land #142924 without adding lockfiles first, and trying to do this work in parallel would cause conflicts, so I'm just going to try to it all in one PR.