Remove unsupported yarn install option. Default to use Yarn berry

[saurori]

Resolves this Issue: #95

Yarn 2+ (berry) is not supported on buffalo new. These changes allow for Yarn 1.x (classic) and 2.x+ (berry) to work, and also defaults to using the latest Yarn berry on setup.

[sio4]

Please check some comments on the code and please take a look at the comment on the original issue #95

[saurori]

@sio4 I pushed fixes addressing your comments 62585b4. I also added enableGlobalCache: true to the .yarnrc.yml file as I think it's a good default to avoid duplicating dependencies across projects.

The resulting .yarnrc.yml file after running buffalo new looks like:

enableGlobalCache: true

logFilters:
  - code: YN0013
    level: discard

yarnPath: .yarn/releases/yarn-3.2.0.cjs

[sio4]

Thank you for your update @saurori ! I think we need to check the .dockerignore file part, meanwhile, I will check the updated branch and will add a comment soon (maybe tomorrow)!

[saurori]

@sio4 any other changes? Are you able to approve the workflow (tests)?

[saurori]

I'm not sure what is causing tests failures:

=== RUN   Test_InstallOldBuffaloCMD/existing_version
    testhelpers_test.go:63: 
        	Error Trace:	testhelpers_test.go:63
        	Error:      	Received unexpected error:
        	            	unknown gobuffalo cli version v0.18.1
        	Test:       	Test_InstallOldBuffaloCMD/existing_version
=== RUN   Test_InstallOldBuffaloCMD/existing_old_version
    testhelpers_test.go:63: 
        	Error Trace:	testhelpers_test.go:63
        	Error:      	Received unexpected error:
        	            	unknown gobuffalo cli version v0.16.27
        	Test:       	Test_InstallOldBuffaloCMD/existing_old_version

[sio4]

Sorry for the delay. I was somewhat busy. It seems like testing was done now. I will check if there is something more and will also check the test failure.

[sio4]

LGTM

[sio4]

I found the reason for the failure by PRing #110. The issue was, I guess, the underlying API call for the test case has a rate limit and we sent too many API calls to api.github.com. Working on it.

[sio4]

This could be tested again once #110 is merged into development.

[paganotoni]

@saurori can you please rebase your branch with development? Thanks in advance

[saurori]

@paganotoni rebased

[saurori]

I am not 100% sure why this failed:

=== RUN   TestBuild/skipop
...
DEBU[2022-03-10T21:34:17Z] Exec: yarnpkg install
        DEBU[2022-03-10T21:34:17Z] ➤ YN0028: The lockfile would have been created by this install, which is explicitly forbidden.
DEBU[2022-03-10T21:34:17Z] ➤ YN0000: Failed with errors in 0s 10ms

Running integration tests locally passes. The error doesn't make sense for a first time yarn install unless immutable builds is set to true. The install command isn't using the --immutable flag, so the only other way that setting is enabled is via env var YARN_ENABLE_IMMUTABLE_INSTALLS=true which means it won't install without a yarn.lock file.

DEBU[2022-03-10T21:34:14Z] LookPath: yarnpkg indicates that yarn is present and does not need to be installed. I wonder if the version of yarn used is the problem? Both the mac and ubuntu github virtual environments list Yarn 1.22.17

My local Yarn:

$ yarnpkg --version
2.4.3

[sio4]

I also am not sure currently. From the log, I can see the following line from the failed test (for ubuntu):

2022-03-10T21:34:31.0041349Z YN0000: Retrieving https://repo.yarnpkg.com/3.2.0/packages/yarnpkg-cli/bin/yarn.js

This output format (YNnnnn) is the format of the berry so I think the running command is the berry. I guess you mentioned version 1.22.17 since the version is on the test log. If you scroll up the log, then you can find the test is for 0.18.0 which is the original version before your PR.

2022-03-10T21:35:10.5828054Z === RUN   TestFix_v0_18_0/web
<...>
2022-03-10T21:35:11.7651292Z DEBU[2022-03-10T21:35:11Z] yarn install v1.22.17

I ran the failed test with your recent branch and it worked fine, so I guess there could be a side effect of the other test or test sequence. I am trying to find the missing point or if the environment or test case itself has any issue. While I was testing it, I found one bug related to .yarnrc.yml (logFilters) and I guess it could be related to the issue. Also, I have a suggestion to make the code simple and straight. I will update them as soon as possible.

[sio4]

The error doesn't make sense for a first time yarn install unless immutable builds is set to true. The install command isn't using the --immutable flag, so the only other way that setting is enabled is via env var YARN_ENABLE_IMMUTABLE_INSTALLS=true which means it won't install without a yarn.lock file.

Oh, I found the direct cause: As you say, this symptom is about immutable and I found that:

If the --immutable option is set (defaults to true on CI), Yarn will abort with an error exit code if the lockfile was to be modified (other paths can be added using the immutablePatterns configuration setting). For backward compatibility we offer an alias under the name of --frozen-lockfile, but it will be removed in a later release.

This behavior of berry is annoying. It seems like there is no opposite option of --immutable such as --no-immutable. (Could be a good FR point?) but the only way I found is using configuration.

Here is my quick patch which covers immutable issue, yarnrc.yml version issue when system yarn is still classic, and consolidate them in a single function. Could you please take a look at this and try applying it if you want? Or I will PR to your branch later. (I have no enough time today)

diff --git a/internal/genny/assets/webpack/templates/dot-yarnrc.yml.tmpl b/internal/genny/assets/webpack/templates/dot-yarnrc.yml.tmpl
deleted file mode 100644
index ba06a62..0000000
--- a/internal/genny/assets/webpack/templates/dot-yarnrc.yml.tmpl
+++ /dev/null
@@ -1,4 +0,0 @@
-enableGlobalCache: true
-logFilters:
-  - code: YN0013
-    level: discard
diff --git a/internal/genny/assets/webpack/webpack.go b/internal/genny/assets/webpack/webpack.go
index 0c0fe5c..68fbb55 100644
--- a/internal/genny/assets/webpack/webpack.go
+++ b/internal/genny/assets/webpack/webpack.go
@@ -13,6 +13,8 @@ import (
 	"github.com/gobuffalo/genny/v2/gogen"
 )
 
+const YARN = "yarnpkg"
+
 //go:embed templates/* templates/assets/css/_buffalo.scss.tmpl
 var templates embed.FS
 
@@ -41,7 +43,7 @@ func New(opts *Options) (*genny.Generator, error) {
 
 	g.RunFn(func(r *genny.Runner) error {
 		if opts.App.WithYarn {
-			if _, err := r.LookPath("yarnpkg"); err == nil {
+			if _, err := r.LookPath(YARN); err == nil {
 				return nil
 			}
 			// If yarn is not installed, it still can be installed with npm.
@@ -87,7 +89,7 @@ func New(opts *Options) (*genny.Generator, error) {
 }
 
 func installPkgs(r *genny.Runner, opts *Options) error {
-	command := "yarnpkg"
+	command := YARN
 	args := []string{"install"}
 
 	if !opts.App.WithYarn {
@@ -97,11 +99,12 @@ func installPkgs(r *genny.Runner, opts *Options) error {
 		if err := installYarn(r); err != nil {
 			return err
 		}
-		if err := r.Exec(exec.Command(command, []string{"set", "version", "berry"}...)); err != nil {
+		if err := setupYarnBerry(r); err != nil {
 			return err
 		}
 	}
 
+	r.Exec(exec.Command(command, "--version"))
 	c := exec.Command(command, args...)
 	c.Stdout = yarnWriter{
 		fn: r.Logger.Debug,
@@ -123,9 +126,31 @@ func (y yarnWriter) Write(p []byte) (int, error) {
 
 func installYarn(r *genny.Runner) error {
 	// if there's no yarn, install it!
-	if _, err := r.LookPath("yarnpkg"); err == nil {
+	if _, err := r.LookPath(YARN); err == nil {
 		return nil
 	}
 	yargs := []string{"install", "-g", "yarn"}
 	return r.Exec(exec.Command("npm", yargs...))
 }
+
+func setupYarnBerry(r *genny.Runner) error {
+	if err := r.Exec(exec.Command(YARN, "--version")); err != nil {
+		return err
+	}
+	if err := r.Exec(exec.Command(YARN, "set", "version", "berry")); err != nil {
+		return err
+	}
+	// the .yarnrc.yml format is different between classic and berry so
+	// it should be configured after installation (set version)
+	if err := r.Exec(exec.Command(YARN, "config", "set", "enableGlobalCache", "true")); err != nil {
+		return err
+	}
+	// can we enable this only for testing? :-(
+	if err := r.Exec(exec.Command(YARN, "config", "set", "enableImmutableInstalls", "true")); err != nil {
+		return err
+	}
+	if err := r.Exec(exec.Command(YARN, "config", "set", "logFilters", "--json", `[{"code":"YN0013","level":"discard"}]`)); err != nil {
+		return err
+	}
+	return nil
+}

[sio4]

I made a PR to your branch (saurori/cli/pull/1) Please take a look at the PR.

[saurori]

@paganotoni if you could re-run the CI with the latest change, thanks!

[sio4]

Please consider the situation when if user's version is classic.

[paganotoni]

Seems to be passing the tests now 🥳 :)

[sio4]

Yeah, to be clarified, the routine added now into cli is only for the new app with berry, so this is not about compatibility with the classic version. The yarn config set ... command will always be run with berry but yarn set version ... could be invoked with the classic version if the user's global yarn is classic.

The classic version had no version set version before. It was added to support berry installation from classic [1] and the command will overwrite (existing) .yarnrc.yml [2] so the template will be ruined. This is the reason I asked to use yarn config after the new .yarnrc.yml created by both versions of yarn.

[1] yarnpkg/yarn@f381c5f
[2] https://github.com/yarnpkg/yarn/blob/6db39cf0ff684ce4e7de29669046afb8103fce3d/src/cli/commands/policies.js#L166:L170

[sio4]

lgtm!