feat(remix-dev): add support for Yarn 3 PnP

[cmd-johnson]

Yarn 3 has a feature called Plug'n'Play (or PnP), which, among other things, gets rid of the node_modules directory.
Unfortunately, the way Remix modifies the remix package when running remix setup node is not compatible with PnP, because it tries to add/edit files in the node_modules directory.
PnP also enforces that all dependencies by a package are included in their respective package.json file, even when the package already indirectly depends on them. Additionally, esbuild doesn't work with PnP out of the box.

I have made a few changes that should enable Remix to work with projects that are using PnP, which I have outlined in #683.
Builds using yarn 3.2.0 (which is currently in the canary stage) now work without having to disable the PnP feature.

What doesn't work though, is using the remix package. As far as I can tell, there's no easy way to change this.
For now, you can work around this by importing from the @remix-run/node, @remix-run/server-runtime,@remix-run/react, ... packages directly.

What could work, is to include everything the remix package does into the project directory. This could also be automated by e.g. adding a flag to the remix setup command. That way, you would only have to change the import {} from "remix"; imports to import {} from "./remix";. In TypeScript projects, the auto-generated files could also be added to the paths property in the tsconfig.json, which means you wouldn't have to change the imports at all.

I'm open for suggestions regarding the remix package issues with PnP and happy to implement them, if that will get PnP support added to Remix.

[cmd-johnson]

I chose to not further go down the path of getting the remix package to work with PnP.
PnP doesn't support the paths property in the tsconfig.json, and will report it as an undeclared dependency. (This also applies to imports from ~/.)

While copying all those files into a sub-directory could still work, I think it's easier to just accept that you'll need to import most things directly from @remix-run/react instead of the remix package.

I'm working on extending the create-remix package with an option for selecting the package manager. The resulting projects will then include all required dependencies to make Remix work with PnP, as well as changing all the imports from the remix package to their respective @remix-run/* packages.

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[cmd-johnson]

Quick update:
These changes don't work with the current version of yarn (v3.1.1), because of yarnpkg/berry#3687.
The issue was already solved by yarn, but is not released yet. It should find its way into the v3.2.0 release.

I'll continue working on this PR once the next yarn version is released.

[badeAdebayo]

is there a chance these changes can get merged anyway. for people who are using yarn 3.2.0?

[cmd-johnson]

Yarn 3.2.0 has just been released! 🎉
I'll try and get around to updating this PR tomorrow.

[cmd-johnson]

From my tests with Yarn (with and without PnP enabled), almost everything appears to be working fine.
What doesn't work are the dev scripts created by create-remix. Those use npm-run-all to run commands in parallel, which fails on my machine. I'm guessing that's because of npm-run-all not being compatible with PnP.

[chaance]

I'm going to close this for the time being. We may revisit this down the road but we have some other priorities we need to focus on before we overhaul our repo tooling, as Yarn 3 is a pretty big shift from how we're doing things at the moment.

[cmd-johnson]

Thank you for your response!
I'm not 100% sure what you mean, because I'm not changing anything about Remix's repo tooling in this PR. These are just changes that allow users of Remix to utilize yarn PnP and doesn't influence how Remix itself is built.

It's mainly about declaring all direct and indirect (peer-)dependencies in all packages in order for yarn to stop complaining.

The only actual changes to Remix features are

• the addition of an (optional) peer dependency on @yarnpkg/esbuild-plugin-pnp that non-PnP users don't need to bother with. As long as the esbuild parameters cannot be customized by users, there unfortunately is no way around this if you want to allow PnP to work with Remix.
• adding an option to use yarn to create-remix

If you think that adding a yarn option to create-remix is too much, I'd be happy to just add a yarn example to the example directory instead.

[chaance]

@cmd-johnson Thanks for clarifying, I must have misunderstood the intent here! Re-opened 👍

[badeAdebayo]

@cmd-johnson now that this is reopen do you plan to fix the conflicts and get this merged?

[cmd-johnson]

They seem to change quite a few things around in the past couple of weeks. I just did that 4 days ago! 😄
I might get to it tomorrow.

Also I think I'd rather go the route of just adding a yarn example over adding an option for it to create-remix. That should make this PR easier to approve and merge.
…and also reduce the number of future merge conflicts. They appear to do a lot of work on create-remix atm.

[chaance]

@cmd-johnson we made a pretty big impactful change by updating our prettier config. A simple way to reduce a lot of the merge conflicts might be to copy/paste the new config and run yarn format from the project root. #2085

[cmd-johnson]

There we go!
I've added an example that's using Yarn 3.2.0 and PnP and removed the "Yarn" and "Yarn PnP" options from the create-remix command again.
That should reduce the overall complexity of this PR.

[badeAdebayo]

will this be merged soon?

[machour]

@cmd-johnson would you mind fixing conflicts and applying @MichaelDeBoey suggestions? 🙏🏼
I'll get some attention to this PR.

[pcattori]

Thanks for all the hard work on this! I plan on discussing the optional dependency stuff with the team this week.

[pcattori]

After talking with the team, here is what we envision:

• We'd like to support the major package managers, including Yarn PnP
• We don't mind adding dependencies to the compiler. We're internally discussing changes to how we want to bundle the compiler and/or CLI that should address any concerns about dependency bloat. So don't worry about it for this PR.
• We'd prefer that support for Yarn PnP work out-of-the-box without requiring users to install a dev dependency

I think that means we can just add @yarnpkg/esbuild-plugin-pnp as a dependency of @remix-run/dev (not an optional peer dependency) and unconditionally add it to our list of esbuild plugins. Since the pnp plugin is added at the end of the list, it should not be a breaking change for apps/projects that want to use normal Node-resolution for their packages.

---

One hesitation I had about simply always including the esbuild pnp plugin is if we will need to conditionally use different resolutions/plugins for different package managers, e.g. pnpm. If we need different plugins for Yarn PnP and for pnpm, then we would need someway to conditionally choose which plugin to use or to automatically detect the package manager being used. We do have getPreferredPackageManager, but that doesn't distinguish between yarn and Yarn PnP.

Is there a good way to automatically detect if Yarn PnP is being used?

If not, I think we'd prefer some config setting over detecting the presence/absence of @yarnpkg/esbuild-plugin-pnp to determine what plugins to use. Hopefully 🤞 we can either setup our plugins so they work across all package managers OR at least autodetect the package manager instead.

---

I also had a couple questions about the code, but I'll leave those as comments on the code rather than within this higher level feedback.

[pcattori]

What does this line do? I was expecting to see an import {...} from "~/..."; somewhere in the example codebase that exercises this, but maybe I'm misunderstanding the purpose of ~ and link:

[SagnikPradhan]

You are right, link: protocol aliases the ./app directory to ~

[cmd-johnson]

PnP doesn't know or care about tsconfig.json files. In order to use the ~ alias that the other examples all provide through their tsconfig.json, we can use the link: protocol that yarn supports to remedy this issue. (See also https://yarnpkg.com/features/protocols#why-is-the-link-protocol-recommended-over-aliases-for-path-mapping)

[pcattori]

Ok that makes sense, but I still can't find anywhere in the examples/yarn-pnp code where the ~ is actually used.

If its not used at all, could we remove this alias from the example to keep things minimal?

[cmd-johnson]

@pcattori

Is there a good way to automatically detect if Yarn PnP is being used?

The way esbuild-plugin-pnp does it is by checking if require("module") has a findPnpApi property.
After a quick test, that property is only there if the PnP feature is used. As soon as I disable PnP and re-install all dependencies, the property is gone.

Since the pnp plugin is added at the end of the list, it should not be a breaking change for apps/projects that want to use normal Node-resolution for their packages.

As far as esbuild-plugin-pnp is concerned, the plugin basically becomes a no-op when PnP isn't used because of the check I linked above.

[MichaelDeBoey]

the plugin basically becomes a no-op when PnP isn't used

Good news, as this means we can put @yarnpkg/esbuild-plugin-pnp in dependencies instead of having it in peerDependencies.

[pcattori]

@cmd-johnson : changes to the Remix compiler look good!

I think there are two main things left to do: (1) address @MichaelDeBoey 's comments for the example and (2) write an integration test that successfully builds a Yarn PnP / Remix app.

...then I think we should be good to merge this after.

[cmd-johnson]

@pcattori I've included the change requests and rebased on the dev branch again.

Re: integration tests: any pointers how I should go about implementing them?
Especially given that I would have to install dependencies of the test project using a different version of yarn than the tests themselves would be running with

[pcattori]

@cmd-johnson : good point, figuring out how to test this is tricky. I'll think more about this today and get back to you.

[pcattori]

Going to merge this as-is without a test since its relatively simple / low-risk. If for whatever reason other changes break Yarn PnP support more than once or twice, we can revisit how to add a test.

Thanks for everyone who participated in this PR and special thanks to @cmd-johnson for the work (and patience!).

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-1e32bfa-20220622 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!