Update yarn.lock

[stephanwlee]

The change includes instruction on how to manage our dependencies.

5252a1f updates the yarn.lock but seems to not contain changes
that would be created if done yarn add. This change manually is
generated by running yarn.

Note that it was not possible to reproduce 5252a1f.

[caisq]

This sentence is a little hard to parse and understand.

yarn.lock will change as a result of adding new dependencies or explicitly upgrading dependencies. Its change should be committed, right? This sentence seems to imply that the change should never be committed.

[caisq]

BTW, thanks for adding this guideline.

[wchargin]

Would it be possible to add a CI check for this? Since lint-frontend
already needs to run yarn to run Prettier, perhaps it could also check
that the Git status of yarn.lock is clean? Otherwise, this seems like
the kind of thing that people will frequently forget to do.

[nfelt]

Is it true that the yarn.lock diff only ever arises when package.json changes, or does it arise sometimes from e.g. a change in our dependencies, or a new release of a dependency that we don't have pinned to a specific version?

If the latter I'm a bit hesitant to make this a CI check as-is, because it seems like it would fail frequently at master with no changes from us, even though it doesn't really indicate a problem (versus just a chore we need to attend to). Also it would make people inclined to update yarn.lock as part of an unrelated change which is bad for blame, etc. It would seem better in that case to try to make the check conditional on the PR touching package.json or yarn.lock.

[stephanwlee]

Yes, it can change.

I think the best solution to move with is to use --frozen-lockfile option on yarn install so it uses the lockfile as the source of truth (frozen-lockfile should be default unless you are meaning to update the dependencies; or at least that was the semantics on npm and its shrinkwrap).

Unfortunately, the version of bazel_nodejs we are using does not support any arguments to yarn, but we can use yarnrc (forgot what it was exactly called) to add one. Want me to alter the PR into that direction instead?

I would not mind if you raise the PR instead.

[wchargin]

At least in normal JS development, when the repository has an existing
lockfile and package.json has not changed, running yarn is
guaranteed not to update the lockfile.* This must be the case, because
the first thing that you do when cloning a JS repo is to run yarn, and
if that dirtied the lockfile then the kind of problem addressed by this
PR would be commonplace. Removing node_modules and running yarn to
re-populate it is also a common operation (at least among people like me
who routinely poke into node_modules for debugging), and that’s also
safe.

* Unless you’re using a newer version of Yarn to run yarn than you
did when you originally generated the lockfile, in which case Yarn may
add integrity checks and stuff as migrations, but that’s kind of a
one-time thing and not a routine occurrence.

We definitely should make sure that this is hermetic, and definitely
shouldn’t update yarn.lock in unrelated commits. I think that the only
way that this could not do what we want is if the Bazel/rules_nodejs
integrations have some behavior that we don’t want, but Bazel is all
about hermeticity, so I wouldn’t expect that to be the case.

(@stephanwlee can correct me if any of this understanding is wrong.)

Gating on package.json/yarn.lock changes is certainly fine with me
if it’s necessary for some reason, and also okay with me if it’s not
strictly necessary but keeps us more comfortable, though it is a bit of
extra code that requires a bit of care to get the right diff range.

[stephanwlee]

At least in normal JS development, when the repository has an existing
lockfile and package.json has not changed, running yarn is
guaranteed not to update the lockfile.*

Not sure if that is entirely true. IIRC, that is correct on npm but not on yarn:
yarnpkg/yarn#4147
yarnpkg/yarn#4379

[wchargin]

It looks like that issue is discussing lock files that don’t accurately
reflect package.json, though? By “package.json has not changed” I
meant “…since the last run of (the same version of) yarn”. Running
npm install --save lodash breaks that condition by changing
package.json without running yarn. Or am I misunderstanding?

(The issue reference is nice, though; maybe adding --frozen-lockfile
to the yarn install earlier in CI is all that we need?)

[stephanwlee]

I attached another GH issue that is more pertinent to our issue. PTAL.

[wchargin]

Interesting, thanks. I wasn’t aware of that. It seems to me that that
issue makes it more important to have this check in our CI. As far as
I can tell, that issue is solely about idempotency, not hermeticity.
That is, it sounds like Yarn eventually converges to a fixpoint of the
lockfile (specifically, after at most 2 steps) that depends only on
package.json and not on the actual non-hermetic contents of
dependencies. So having it in CI ensures that we’ve reached that
fixpoint, and doesn’t introduce flakiness because the correct output is
always determined by the repository contents. Is that right? (I mean,
I’m prepared to be wrong again! :-) )

(For what it’s worth, I haven’t personally run into either of these
issues on SourceCred or any personal projects.)

[wchargin]

Perfect; thanks! Extra points for the extra safe -- yarn.lock form,
and thanks for verifying that it fails at head.

[stephanwlee]

The approach is generally flawed. Abandoned.