Remove obsolete yarn.lock files and check in root yarn.lock file

[rmacklin]

Resolves #34587

Issue description

Steps to reproduce

cd actioncable
yarn add --dev left-pad

Expected behavior

There should be changes to yarn.lock that are tracked by git.

Actual behavior

There are only changes to package.json tracked by git - no changes to yarn.lock tracked by git

System configuration

Rails version: N/A

Ruby version: N/A

Analysis

This seems to be because the repository is now using Yarn Workspaces (added in #33079):

rails/.yarnrc

Lines 1 to 2 in a429b29

	workspaces-experimental true
	--add.prefer-offline true

rails/package.json

Lines 3 to 10 in a429b29

	"workspaces": [
	"actioncable",
	"activestorage",
	"actionview",
	"tmp/templates/app_template",
	"railties/test/fixtures/tmp/bukkits/**/test/dummy",
	"railties/test/fixtures/tmp/bukkits/**/spec/dummy"
	],

This makes the yarn.lock files inside the actioncable and activestorage directories obsolete. Now the only yarn.lock that matters is in the repository root. However, that file is currently in the .gitignore:

rails/.gitignore

Line 19 in a429b29

/yarn.lock

This seems undesirable. If you have a stale yarn.lock in the root, it won't be modified to add new packages when you git pull, even if new dependencies have been added in one of the workspaces. This means that when you run yarn install, you won't have all the dependencies installed. You have to first run rm yarn.lock to remove your stale lockfile, and then run yarn install to get everything installed. Since the root yarn.lock is currently in the .gitignore, it's easy to miss that you have this file locally, so it's not obvious that you should run rm yarn.lock before yarn install.

I think we should remove /yarn.lock from the .gitignore and check in the root yarn.lock like we check in the root Gemfile.lock.

Related thread: #34370 (comment)

[javan]

👌 This is the correct way to use Yarn Workspaces, AFAIK. We have the same setup for https://github.com/stimulusjs/stimulus: a single yarn.lock at the root, and individual package.jsons for each workspace.

[javan]

Let's make sure yarn.lock is up to date and CI is happy now that #34370 is merged. Can you please rebase and run yarn install once more?

[rmacklin]

#34370 didn't add any dependencies, so there are no changes when I rebase and run yarn install. Do you want me to push that anyway?

[javan]

Yes, please. I want to make sure actioncable's tests still pass on CI. Can't hurt to rebase.

[rmacklin]

Sounds good. It's rebased and passing

[javan]

When I run yarn install, I get a pretty big yarn.lock diff: https://gist.github.com/javan/1e3b5d9805a5ac072ee9c12ea0bc43e6

[rmacklin]

Hmm, do you know what these entries are for in the workspaces configuration:

rails/package.json

Lines 7 to 9 in aa1ba9c

	"tmp/templates/app_template",
	"railties/test/fixtures/tmp/bukkits/**/test/dummy",
	"railties/test/fixtures/tmp/bukkits/**/spec/dummy"

After running

rm -r tmp/templates/app_template railties/test/fixtures/tmp

to delete those directories and rerunning yarn install, I get a diff as well (wasn't able to get a diff before doing that).

It seems like these directories are also untracked by git which means that you could have stale files in there which cause the root yarn.lock to be generated differently. I'm inclined to think they shouldn't be configured as workspaces, but I don't have all the context to know for sure.

For now, I'll push the yarn.lock that was generated after removing those directories from my local environment.

[rmacklin]

It seems to have originated from this commit:
lsylvester@b801b08 (part of #33640, a sub-PR of the larger #33079).

@lsylvester commented in #33640 (comment) about some tests potentially requiring the workspaces configuration, but I'm not sure if that included these lines

rails/package.json

Lines 7 to 9 in aa1ba9c

	"tmp/templates/app_template",
	"railties/test/fixtures/tmp/bukkits/**/test/dummy",
	"railties/test/fixtures/tmp/bukkits/**/spec/dummy"

or if

rails/package.json

Lines 4 to 6 in aa1ba9c

	"actioncable",
	"activestorage",
	"actionview",

is sufficient.

@lsylvester, if you're able to provide more context that'd be great. Otherwise, I would suggest we try removing the tmp directories from the workspaces configuration and see if the tests pass.

[javan]

+1 for removing those entries. Test / tmp directories shouldn't be workspaces.

[lsylvester]

HI @javan, @rmacklin,

I can provide some context as to why I added the tmp directories into the workspaces.

At the time - we were getting errors in the railties tests that generated applications and then needed to compile the assets as the versions that the were being required (which were based on the Rails::VERSION hadn't been published.

I added in the yarn workspaces in order to allow the Rails application that were generated in the tmp directory visibility of the different required packages. Without the tmp directories in workspace the generated rails applications didn't have visibility of the unpublished packages.

Since then, the version of the packages that is required was relaxed (fbd4597) to include published versions. This was done after a discussion about the differences in the versioning schemes between npm and ruby gems where Rails::VERSION::STRING could not be directly used.

I am not sure what version of the packages was being used in the tests prior to this being merged (ie latest on npm or alpha version that is in the rails repo) but I think that without the tmp directories in the workspace, it will be the 5.2.X versions. I don't know if this is an issue as long as any issues get picked up in the prerelease. All the railties tests are doing are ensuring that the packages can compile, so I think this is unlikely to be an issue unless something is seriously broken.

I don't think that the workspaces are currently providing any value, but expect that that might change once ActionText is merged, as it depends on the ActiveStorage package.

I hope that that clears some things up.

[rmacklin]

Thanks a bunch for the context! Given that:

Since then, the version of the packages that is required was relaxed (fbd4597) to include published versions. This was done after a discussion about the differences in the versioning schemes between npm and ruby gems where Rails::VERSION::STRING could not be directly used.

it seems like you agree that we can remove the tmp directories from the workspaces. I opened a PR to do that here: #34630

I agree that the workspaces should provide value when ActionText is merged, so I did not remove the workspaces configuration for the real actioncable, actionview, and activestorage packages.