Warn if a developer is using yarn@2 with PnP

[samtstern]

Description

See #2198

Scenarios Tested

A basic yarn setup (provided in #2198) with .pnp.js for dependencies.

Sample Commands

N/A

[abeisgoat]

This is pretty scary junk, can you add some comments about what's going on?

[samtstern]

@abeisgoat good idea, many comments added.

BTW I am very open to the verdict that yarn2 support is not worth it, but since I spent basically a whole day figuring this stuff out I figured I'd post the PR and we could discuss.

[walsquared]

Could this PR be re-reviewed? It would be lifesaver for a project I'm working on.

[samtstern]

@Walsker would you mind trying this branch and seeing if it actually works for your project? If it does I'm inclined to release this.

Here's how to do that:

# Clone this repo
git clone https://github.com/firebase/firebase-tools
cd firebase-tools

# Checkout the branch
git checkout ss-fix-yarn-2

# Build the repo locally
npm install
npm run build

# Make the `firebase` command on your system point to the local build
npm link

Once you do all these steps the firebase command on your system will point to your local build. After you're done testing you can just do npm install -g firebase-tools@latest to go back to a public build.

[walsquared]

My findings, testing firebase-tools on a firebase-functions project

Development

Javascript: works great
Typescript: there seems to be some types packages that cannot be found when trying to build, namely

• firebase-functions depends on express-serve-static-core
  • I tried fixing this one using Yarn 2's package extensions, however the dependant package wasn't able to find any of the exports it needed from it.
• Many sdk packages could not find node types
  • Just explicitly installing @types/node as a dev dependency in my project solved this, but I imagine there's a dependency magic that's happening in those packages that Yarn 2 is now catching
• Something is installing a second @types/node at a different version, and firebase-admin is facing a conflict when picking one
  • Just explicitly installing @types/node as a dev dependency fixed this too
• @firebase/app-types has a peer dependency on @firebase/logger
  • Normally this wouldn't be an issue, but with yarn 2 I believe this has to be explicitly defined. Couldn't find a way to solve this

I'm sure it's possible to patch these dependencies on the cli side, however the main issue is likely that these packages have their dependencies configured in a way that works okay with a node_modules folder, but not with PnP. As a last resort I enabled PnP loose mode (not desirable) which solved all issues except the conflicting node types issue above.

I tried the same typescript files with npm and yarn 1, both worked fine. I also confirmed that these issues were present on a fresh project configured to typescript.

Emulation

Javascript: works fine
Typescript: works fine once you can build

Deployment

It seems that the deployment process uses npm regardless of whether you chose not to manage your packages with npm when initializing. In our case, this mean that the deployment process has no knowledge of where any packages are, so it fails any npm command it tries to run.

Additional Notes

This solution doesn't support yarn workspaces, where the .pnp.js file might not be alongside the package.json file. Although, it seems that firebase-tools in general is still working on workspaces support, so perhaps this problem is out of the scope of this PR.

Unfortunately my project uses typescript, and with the addition of firebase functions I would be looking to configure it as a workspace, so I think I hit a double whammy of incompatibility 😅

[samtstern]

@Walsker thank you for trying it and leaving such detailed feedback! To be totally honest the issues you've discovered go way beyond my personal knowledge of Yarn/PnP and I'm not sure how I would address them. I don't think anyone on the team knows much more about it either, since we don't use Yarn2 for anything internally right now.

You also raised an important point:

It seems that the deployment process uses npm regardless of whether you chose not to manage your packages with npm when initializing. In our case, this mean that the deployment process has no knowledge of where any packages are, so it fails any npm command it tries to run.

This is true and will remain true for the foreseeable future. Which leads me to believe that supporting Yarn2 locally could be a bad thing. The point of these emulators is to make local development simple so that you can confidently deploy to production. If something works locally but won't deploy or won't work correctly after deploying we've actually done harm!

So I think rather than try to patch all of these holes we should instead just say "Cloud Functions for Firebase does not work with Yarn2". We can use some of the things in this PR to give clear warnings to developers who try to use Yarn2 now that we know how to detect it.

What do you think about that? Am I missing something?

[walsquared]

No you're totally correct, patching all of this would be a lot. In terms of how you say that last part, it's worth noting that firebase-tools is incompatible with Plug n' Play, not Yarn 2 itself. PnP is still a super new feature, and Yarn themselves says it's optional in their migration guide.

I ended up enabling the node_modules folder in my Yarn 2 project (see config), which avoids this whole situation altogether. I haven't seen any issues or differences since I enabled it. I would recommend people take this path if they are eager to use Yarn 2.

[samtstern]

@Walsker glad we're on the same page and thanks for clarifying the terminology. I'll update this PR to be more of a "detect and warn" thing.

[barbogast]

@samtstern The file is called .pnp.cjs for new versions of yarn2. Would be good to include the new name, I didn't see the warning until I tried the deployment.

[samtstern]

@barbogast would you like to send a PR to fix this? I guess we should check for both.