fix: fix the builds for public projects that use yarn 2 (or above) with workspaces

[fulopkovacs]

Summary

Fixes #762

The problem it solves

Currently builds are failing for public projects that use yarn 2 (or above) with workspaces. The cached node modules are cached and restored to the wrong location.

The bug

A bit of context: in yarn 1.x only private repos can have workspaces, but this limitation was removed in yarn 2:

Worktrees used to be required to be private (ie list "private": true in their package.json). This requirement got removed with the 2.0 release in order to help standalone projects to progressively adopt workspaces (for example by listing their documentation website as a separate workspace). (source: docs)

The bug is in the run-build-functions.sh file:

  # YARN_IGNORE_PATH will ignore the presence of a local yarn executable (i.e. yarn 2) and default
  # to using the global one (which, for now, is always yarn 1.x). See https://yarnpkg.com/configuration/yarnrc#ignorePath
  # we can actually use this command for npm workspaces as well
  workspace_output="$(YARN_IGNORE_PATH=1 yarn workspaces --json info 2>/dev/null)"
  workspace_exit_code=$?

As you can see it tries to output information about the workspaces with yarn 1.x. in the command substitution and redirects STDERR to /dev/null. After that it only checks if the process exited with 0 or an error code, but ignores the error message, which would tell us, that "workspaces can only be enabled in private projects". Here's an example for the problem from the theatre-js/theatre repo:

• when the errors are hidden:

 $ YARN_IGNORE_PATH=1 yarn workspaces --json info 2>/dev/null
{"type":"info","data":"Visit \u001b[1mhttps://yarnpkg.com/en/docs/cli/workspaces\u001b[22m for documentation about this command."}

• when the errors are visible:

$ YARN_IGNORE_PATH=1 yarn workspaces --json info
{"type":"error","data":"Workspaces can only be enabled in private projects."}
{"type":"info","data":"Visit \u001b[1mhttps://yarnpkg.com/en/docs/cli/workspaces\u001b[22m for documentation about this command."}

The solution

• We can capture the error messages instead of discarding them to check if the problem is related to private workspaces.
• If it is, then we check the version of yarn that is used in the project and use the new yarn workspaces list --json command to get the information about the workspaces

How I tested the solution

We have a public repo whose builds are failing without clearing the cache after every build. Here's how the cached folders look like with the current build script:

The yarn workspaces are not detected in the logs:

But here's the previous pictures look like for the fixed build script:

I'm not sure if I missed anything, but feel free to tell me if there are things to change! ☺️

---

For us to review and ship your PR efficiently, please perform the following steps:

• Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
• Read the contribution guidelines 📖. This ensures your code follows our style guide and passes our tests.
• Update or add tests (if any source code was changed or added) 🧪
• Update the included software doc (if you updated included software) 📄
• Update or add documentation (if features were changed or added) 📝
• Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

[fool]

added to backend/support pairing - @audreyshub could you please raise this with the next person you meet with to ask the relevant team to review this for us?

[JGAntunes]

Thank you for your contribution @fulopkovacs! 🎉 And thanks @fool for raising this. I've added this to our board so we can triage it and prioritise the review next Monday during our planning session.

[JGAntunes]

Apologies for taking so long to review this @fulopkovacs and thank you for your contribution!

Overall the approach looks good, however I think we could simplify this and reduce the number of code branches if we simply checked for the installer and the yarn version up ahead. Let me know what you think.

I was also wondering how would you feel about adding an automated test for this? You can rely on the helper functions we have to setup a fixture workspace in order to validate this:

• build-image/tests/node/yarn.bats

  Line 16 in 122eea9

  set_fixture_as_repo 'simple-node' "$TMP_DIR"

[JGAntunes]

Since we know the installer up ahead and we can check for yarn's version (similar to what we're doing in here already) I was thinking we could simplify things here a bit and not require us to check the error logs of the first command execution? We could instead check:

• if the installer is yarn and we're using version greater than 1, run the remainder of this then block
• else run the YARN_IGNORE_PATH=1 block.

Something like (nvm the chaos and indentation):

local package_manager_version
package_manager_version=$(yarn --version)

if [ "$installer" = "yarn"] && [ "${package_manager_version:0:1}" -gt 1 ]
then
  # the yarn berry workspaces logic
  (...)
else
  (...)
  workspace_output="$(YARN_IGNORE_PATH=1 yarn workspaces --json info 2>/dev/null)"
  (etc.)
fi
# Handle the output of the executions above
if [ ${#package_locations[@]} -ne 0 ]
  then
    echo "$installer workspaces detected"
    # Extract all the packages and respective locations.
    restore_js_workspaces_cache "${package_locations[@]}"
  else
    echo "No $installer workspaces detected"

[fulopkovacs]

@JGAntunes Hey 👋,

sorry for replying 4 days late, I forgot about this issue... 😅 Your suggestion seems to be reasonable, I'd be happy to try it and add add a test this week, if that's okay for you (tbh I might have to work on it on the weekend, because currently it doesn't fit into our schedule at work).

[JGAntunes]

@fulopkovacs sure thing, however works better for you 👍 again, apologies for taking so long to follow up.

[fulopkovacs]

@JGAntunes Shipping a binary for a specific version of yarn might be controversial, but

1. it's a valid real-world use case (we use this configuration at work)
2. it's faster to run the tests, because we don't have to download a new yarn version.

[fulopkovacs]

@JGAntunes I finally pass the PR back to review! 😁 There were a few surprises with the tests (e.g.: many of the tests in yarn.bats executed the run_yarn function, which removes the default yarn installation, but there was no proper cleanup after these tests, so the binary for yarn was not available for tests in other files).

One of our biggest pain point gets addressed, if this PR gets merged, but there will still be the problem of caching, which doesn't really work with yarn berry now (35-45s for us for every deploy). Are there any (public) news about that? I saw a PR about it (#465), but it didn't receive much attention from your side (I didn't check the code of the PR though).

(Btw having worked with the code base and seeing the tests I understand that full support for yarn berry would require quite a bit of planning and effort, so I don't expect you to fix the issue overnight. I would be happy with just a bit more clarity about the place of this feature on the roadmap ☺️ )

[lukasholzer]

Support got already implemented inside #858