Skip to content
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

Remove yarn check #106

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Remove yarn check #106

merged 1 commit into from
Feb 18, 2019

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Oct 30, 2018

Cf document for motivations; in short this command is of little to no use to us, but contributes to our users being confused by its buggy behavior.

@cprussin
Copy link

cprussin commented Oct 30, 2018

@arcanis our current use case for yarn check is to catch cases where developers are switching between branches and not running yarn, causing them to write code against library versions that don't match what goes to prod (we run yarn check on pre-push and fail the push if the developer is in an inconsistent state). Without yarn check, what is yarn's suggested mechanism for avoiding these types of issues?

Things we considered:

  1. Run yarn on post-merge. This is impractical because, while yarn is fast, we have a large app with many dependencies and the yarn install is slow enough to be a burden.
  2. Ignore inconsistent states and let CI catch issues. This is impractical because it requires an extremely high bar of automated testing, and it's likely that new code that developers write will have untested components.

How do you guys typically handle this scenario?

cc @betaorbust

@arcanis
Copy link
Member Author

arcanis commented Oct 31, 2018

How do you guys typically handle this scenario?

Right now we simply make our development server restart (and run yarn install again) when they detect that the package.json or yarn.lock is modified.


I think the solution will eventually be in the form of the project-local cache folder. It's currently a work in progress (which will probably be shipped as an experimental feature early next year), but the idea I'm working on is to make Yarn a development-only tool: you wouldn't need to run it at all on production / CI. In fact, you wouldn't have to run it at all except when running yarn add/remove/upgrade.

In order to do this, the cache folder (that would be reworked to only contain zip files) along with the .pnp.js file would have to be checked-in within your repository. By doing this, your developers wouldn't be able to forget running yarn install: they would get the right versions of everything just by switching branches.

@alloy
Copy link

alloy commented Oct 31, 2018

How do you guys typically handle this scenario?

In CocoaPods we solved this by having a manifest of what got installed inside the dependencies directory that CocoaPods would check for and compare to the lockfile in the project during a build.

In the case of Yarn when running a command through yarn [command] I can see it check node_modules for a copy of yarn.lock or a checksum of yarn.lock and compare it to the one in the project.


## 2. Detailed Design

We'll remove the `yarn check` command in Yarn 2.0.
Copy link

Choose a reason for hiding this comment

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

As well as yarn install --check-files, I assume?

Copy link
Member Author

Choose a reason for hiding this comment

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

So --check-files is a bit different - it doesn't actually check 😅

It has to do with the way we make an early bailout if the node_modules is already there (to avoid running an installation if there is no need). Without --check-files Yarn will only checks for the various folders to exist (plus various other settings), whereas with --check-files it will also check for all the registered files to exist. It's not super useful anymore, but it's not as redundant as yarn check.

Copy link

Choose a reason for hiding this comment

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

Oh, I see. That’s unfortunate, I like the integrity check of —check-files, but for most people I can see checking of the expected directories exist is probably enough.

@alloy
Copy link

alloy commented Oct 31, 2018

At Artsy we’ve been telling everybody to run yarn install --check-files all the time, so I’m very much in favour of yarn install ensuring a correct installation by default 👍


## 4. Drawbacks

- The one use of `yarn check` is to provide debug information when installs aren't properly made. In practice it's never used this way, even by maintainers.
Copy link
Member

Choose a reason for hiding this comment

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

Ha true, it'd point out any glaring issues but I'd agree the results aren't reliable at all, esp with workspaces and resolutions

@hibachrach
Copy link

Why isn't this mentioned anywhere on the website or in the yarn repo for 1.x? A little frustrated as we lost a day to this and had to do some sizable reverts.

YourFin added a commit to AlexanderOtavka/ride-board that referenced this pull request Jan 11, 2020
They have been deprecated by yarn:
yarnpkg/rfcs#106
@ndbroadbent
Copy link

Hello,

I would like to add a yarn check stage to my CI builds, and fail if there are any warnings or errors. For example, here are some current issues that I'm fixing:

$ yarn check
yarn check v1.19.2
error "acorn" is wrong version: expected "6.4.1", got "7.2.0"
error "espree#acorn" not installed
warning "jest-environment-jsdom#jest-util#@jest/source-map@^24.9.0" could be deduped from "24.9.0" to "@jest/source-map@24.9.0"
warning "babel-jest#@jest/transform#jest-util#@jest/source-map@^24.9.0" could be deduped from "24.9.0" to "@jest/source-map@24.9.0"
error "espree#acorn-jsx" not installed
warning "babel-jest#jest-util#@jest/console#@jest/source-map@^24.9.0" could be deduped from "24.9.0" to "@jest/source-map@24.9.0"
warning "jest-environment-jsdom#jest-util#@jest/console#@jest/source-map@^24.9.0" could be deduped from "24.9.0" to "@jest/source-map@24.9.0"
warning "@jest/fake-timers#@jest/test-result#@jest/console#@jest/source-map@^24.9.0" could be deduped from "24.9.0" to "@jest/source-map@24.9.0"
error "jest-config#jsdom#acorn" not installed
info Found 5 warnings.
error Found 4 errors.
info Visit https://yarnpkg.com/en/docs/cli/check for documentation about this command.

I read the page at https://yarnpkg.com/en/docs/cli/check, and found this issue. But it looks like yarn install check-files doesn't have the same behavior:

$ yarn install --check-files
yarn install v1.19.2
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
success Already up-to-date.
✨  Done in 2.66s.

Also I'm trying to get some other warnings/errors to show up with a CLI flag, but it seems like I need to run: rm yarn.lock && yarn to get these to appear:

$ yarn
yarn install v1.19.2
info No lockfile found.
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
warning @babel/cli > chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
warning @babel/cli > chokidar > fsevents@1.2.13: fsevents 1 will break on node v14+ and could be using insecure binaries. Upgrade to fsevents 2.
warning @babel/cli > chokidar > braces > snapdragon > source-map-resolve > resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
warning @babel/cli > chokidar > braces > snapdragon > source-map-resolve > urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
warning @babel/polyfill@7.8.7: 🚨 This package has been deprecated in favor of separate inclusion of a polyfill and regenerator-runtime (when needed). See the @babel/polyfill docs (https://babeljs.io/docs/en/babel-polyfill) for more information.
warning @babel/polyfill > core-js@2.6.11: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.
warning babel-jest > @jest/transform > jest-haste-map > fsevents@1.2.13: fsevents 1 will break on node v14+ and could be using insecure binaries. Upgrade to fsevents 2.
warning jest > @jest/core > jest-config > jest-environment-jsdom > jsdom > request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
warning jest-environment-enzyme > jest-environment-jsdom > jsdom > request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
warning jest-environment-enzyme > jest-environment-jsdom > jsdom > left-pad@1.3.0: use String.prototype.padStart()
warning node-sass > request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
warning node-sass > node-gyp > request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
warning rc-slider > babel-runtime > core-js@2.6.11: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.
warning react-on-rails > @babel/runtime-corejs2 > core-js@2.6.11: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.
warning resolve-url-loader > rework > css > urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
warning webpack > watchpack > watchpack-chokidar2 > chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
warning webpack-dev-server > chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
[5/5] 🔨  Building fresh packages...
success Saved lockfile.
✨  Done in 94.72s.

I would like to check for all of these warnings and errors in my CI builds, and fail the build if there are any problems with my package.json or yarn.lock. What would be the best way to do this without using yarn check? (and maybe also without rm yarn.lock)

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.

6 participants