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 integrity check #2518

Merged
merged 3 commits into from
Apr 11, 2020

Conversation

rossta
Copy link
Member

@rossta rossta commented Apr 1, 2020

Problem

The yarn integrity check is often a cause of confusion and frustration for developers: #2517, #2322, #1568

That it is run in an initializer and potentially interferes with any Rails command that boots the environment is also surprising to many: #1135

Yarn's own maintainer discourages its use: yarnpkg/yarn#6427 (comment)

The yarn check command has also been removed from Yarn 2.0.

Solution

Remove the integrity initializer and the configuration options. Encourage developers to ensure yarn install in local environments and yarn install --frozen-lockfile as part of deployment.

@rossta rossta force-pushed the feature/chore-remove-yarn-check branch from 84ee956 to 579fd77 Compare April 1, 2020 01:53
rossta added 3 commits March 31, 2020 21:54
The `yarn check` command has been removed in Yarn 2.0.

The yarn integrity check in Webpacker has commonly been a source of
confusion and frustration among developers. Its behavior at times does
not always match expectation.

Yarn's maintainer has described `yarn check` as buggy and discourages
its use: yarnpkg/yarn#6427 (comment)

This PR removes the yarn integrity check from the Webpacker railtie as
well as references to its setting in Webpacker::Configuration.

The Webpacker::Configuration#check_yarn_integrity= method has been left
in with a deprecation warning to avoid a breaking change.
It's recommended to use `yarn install --frozen-lockfile` in a deployment
context prior to compiling assets for production. This practice may help
offset potential concerns with the removal of the yarn integrity check,
at least in production environemnts.
@rossta rossta force-pushed the feature/chore-remove-yarn-check branch from 579fd77 to 3f6e016 Compare April 1, 2020 01:54
@cabello
Copy link

cabello commented Apr 7, 2020

Thank you for this contribution hoping this will be merged or solve soon, I started working on a project with webpacker a few days ago and I have been constantly seeing this error message despite no changes to Javascript dependencies. It goes away once I run --check-files and it comes back in cycles.

In other projects I do use the same approach frozen lock, not only for yarn.lock but also Gemfile.lock, so this proposal seems reasonable and sane, huzah! 🎉

@gauravtiwari gauravtiwari merged commit 09caeed into rails:master Apr 11, 2020
@gauravtiwari
Copy link
Member

thanks @rossta been thinking to remove this from a while 🍰

ledermann added a commit to ledermann/docker-rails that referenced this pull request Apr 20, 2020
aliuk2012 added a commit to DFE-Digital/publish-teacher-training-2022 that referenced this pull request Apr 21, 2020
Webpacker 5.1.0 removed yarn integrity checker. This commit ensures that
we also use the yarn.lock we generated while developing rather than updating
it. It should also fail if package.json & yarn.lock are out of sync.

Yarn's own maintainer discourages its use:
yarnpkg/yarn#6427 (comment)
and suggests the following solution:
Solution
Remove the integrity initializer and the configuration options. Encourage
developers to ensure yarn install in local environments and
yarn install --frozen-lockfile as part of deployment.

Other references:
https://classic.yarnpkg.com/en/docs/cli/install#toc-yarn-install-frozen-lockfile
rails/webpacker#2518
aliuk2012 added a commit to DFE-Digital/find-teacher-training that referenced this pull request Apr 22, 2020
Webpacker 5.1.0 removed yarn integrity checker. This commit ensures that
we also use the yarn.lock we generated while developing rather than updating
it. It should also fail if package.json & yarn.lock are out of sync.

Yarn's own maintainer discourages its use:
yarnpkg/yarn#6427 (comment)
and suggests the following solution:
Solution
Remove the integrity initializer and the configuration options. Encourage
developers to ensure yarn install in local environments and
yarn install --frozen-lockfile as part of deployment.

Other references:
https://classic.yarnpkg.com/en/docs/cli/install#toc-yarn-install-frozen-lockfile
rails/webpacker#2518
ledermann added a commit to ledermann/pingcrm that referenced this pull request Apr 22, 2020
@jrochkind
Copy link

Encourage developers to ensure yarn install in local environments and yarn install --frozen-lockfile as part of deployment.

Should Rails do a yarn install --frozen-lockfile when you do RAILS_ENV=production ./bin/rails assets:precompile? Will it?

@schmijos
Copy link

Encourage developers to ensure yarn install in local environments

Are you serious?

If I don't run yarn install, things now fail silently after changing to a git branch where yarn.lock is out of sync with my node_modules folder. I think the library's defaults are now worse.

The least you should do in such a PR is to draw a hopeful picture by referencing Yarn 2.0 (Yarn Plug’n’Play)
.

@jrochkind
Copy link

jrochkind commented May 14, 2020

If I don't run yarn install, things now fail silently after changing to a git branch where yarn.lock is out of sync with my node_modules folder. I think the library's defaults are now worse.

Oh no, that's terrible. Right now when switching between development branches, I frequently get the yarn integrity check warning.

If the alternative post this change is silent failing... that is disastrous. This stuff is very hard to debug. Even for "intermediate" devs (with JS) like me -- beginners are going to be absolutely stymied.

I get that if yarn itself is removing the feature what you can do.... but I couldn't remove it from webpacker until yarn removes it, and I would prioritize figuring out another solution that will avoid silent hard to debug failures. I see the point of webpacker as a good default integration that saves you from some of the really difficult parts of the JS toolchain world... this is really falling down, if it's true that it will be silent failures when your node_modules are out of sync with yarn.lock. This definitely does happen a lot when switching between dev branches.

@cabello
Copy link

cabello commented May 14, 2020

We could find a way to detect false positives and address them. I disabled this feature using the configuration flag because I was getting integrity issues despite no changes to dependencies, it would not install anything after yarn install then come back a few commits later.

@rossta
Copy link
Member Author

rossta commented May 15, 2020

@schmijos I feel your pain. Switching branches in any NPM-managed project is a problem. There are a bunch of blog posts about working around this issue such as with git hooks, git worktree, or checking in node_modules, etc. None of those ideas are perfect but it’s not like there aren’t alternatives.

The yarn integrity check didn’t work as advertised. It interfered as often as it helped. The yarn maintainer said it shouldn’t be used. If it worked for you, you could certainly add it back to your workflow.

My opinion is that Webpacker shouldn’t try to solve a weakness in JavaScript package management. But I also want this project to succeed. Since you sound motivated, I’d be interested to see how you could help through a pull request.

@cabello
Copy link

cabello commented May 15, 2020

@rossta thank you for bringing that up, you reminded me to share my solution for this, which is using git hooks https://gist.github.com/cabello/416baa581631779d991b88c265b209aa

It bundle installs if Gemfile.lock changed, it yarn install if yarn.lock changed and a few other goodies. It works quite well for my use case.

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.

5 participants