[core] Use frozen-lockfile by default

[eps1lon]

Revert --frozen-lockfile change from #14050

@rosskevin Didn't explain why he thought this was good. We have a dedicated CI task to check if the lockfile doesn't match package.json. This flag makes this task useless. I doesn't completely.

I would hope that CI and local development use the same dependencies which is what this lockfile is for. This also means that we test with the latest versions of our dependencies not (possibly) outdated versions from the lockfile.
Running yarn locally creates a diff because frozen-lockfile is not default. This enables frozen-lockfile following yarnpkg/yarn#4147.

[oliviertassinari]

@eps1lon You need to rebase on next. This flag looks like a best practice, why do you want to revert it? https://github.com/facebook/react/blob/master/.circleci/config.yml#L32.

[eps1lon]

@eps1lon You need to rebase on next. This flag looks like a best practice, why do you want to revert it? facebook/react:.circleci/config.yml@master#L32.

Then we can remove the CI task to check if the dependencies are up to date and add to CONTRIBUTING that you need to run yarn --frozen-lockfile locally. Otherwise you will get diffs on every yarn install locally.

Edit: Also: best practice for library code is no lockfile and for apps with a lockfile. We mix both in this repo. Having a lockfile allows us to monitor transitive dependencies (which should be the responsibility of every library author IMO)

[oliviertassinari]

I have noticed some occurrences where the flag isn't enough. It still outputs diff in the yarn.lock. I can provide an example.

[oliviertassinari]

Shoulder strap and belt.

[eps1lon]

I have noticed some occurrences where the flag isn't enough.

To do what? What do you want to do. Please don't handwave this again.

[oliviertassinari]

To do what?

To have a truly frozen yarn.lock. But I'm happy to follow React.js test configuration.

[eps1lon]

To do what?

To have a truly frozen yarn.lock. But I'm happy to follow React.js test configuration.

Again this is not rationale. "I'd like to have cookies" is not a sufficient argument to eat cookies all day 😛

But this PR doesn't change that. It even brings that to local machines following best practices: yarnpkg/yarn#4147 and https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all/

[oliviertassinari]

Thanks for the links.

[rosskevin]

I did explain.

This flag was needed because CI failed while locally I succeeded. Given I was up to date with yarn, did a full rebuild of node_modules, and still succeeded locally but failed on CI, this demonstrates a flaw somewhere. The only reliable solution was to build with the lock and keep it up to date. My suggestion was to automate dependency updates with renovate or greenkeeper instead, but Olivier didn't want the PR noise and I can definitely understand that sentiment.

Given my theory about the cache, and what was not answered by @eps1lon in that PR was if we still need the offline cache hack he added? That is what I suggest the only diff between local and CI....and my inclination is to remove this offline cache hack and simplify the cache config.

My builds run on circleci and I need no such hack, perhaps we drop the offline cache and drop the frozen lockfile?

[eps1lon]

The offline-cache is used because the network in CircleCI is not reliable. We had repeated build failures because the network timed out. We didn't have any network issues after this change.

I didn't explain this because it is/was just a reiteration of the original PR/commit message for this change. However you did not explain why this was suddenly not necessary anymore. Do you have a source that described the original problem and that it was fixed?

This flag was needed because CI failed while locally I succeeded.

That is just not a valid explanation. I expect more from contributors that open issues so if I ask you explicitly I expect that you follow a similar schema: "What did you do? What did you expect? What happened? Why do you think this is a bug?"

Following the original commit history it looks like an issue with a dependency of dtslint and more specifically how some yarn versions handle github dependencies. This is almost certainly cause by different versions of yarn i.e. a (fixed) bug in yarn. It does not follow from this that we should change how our CI works.

I'm more than happy to understand why having a frozen lockfile in CI is a good thing when not communicating that to our devs results in different dependency trees for local development and CI.

My builds run on circleci and I need no such hack, perhaps we drop the offline cache and drop the frozen lockfile?

A documented configuration setting that is used how documented is a hack?

I guess I just have to live with not knowing why frozen-lockfile is good. I don't have the intuition that you two seem to have. This was unnecessarily emotional so I'm sorry for that

[jedwards1211]

So, I needed to add @babel/core and @babel/traverse for building a whitelist for my top-level-imports codemod, and now I'm getting the "Your lockfile needs to be updated, but yarn was run with --frozen-lockfile" because of this change. How do I force it to update the lockfile now? Is temporarily turning off that flag in .yarnrc the only way?

[eps1lon]

It's a bug in yarn 1.x as far as I know. Did you install it with yarn workspace @material-ui/codemod @babel/core?

Is temporarily turning off that flag in .yarnrc the only way

TL;DR: yarn v2 will solve this

Yes. It's mainly there so that new contributors don't get a lockfile change on their first install or rather every clone having the same node_modules. We previously checked this different in CI but it also caused some issues with github dependencies.

[jedwards1211]

yeah after I got the error I tried the command you suggested, but same problem. All good, I just wanted to check if there is a better way to do this