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.

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

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.

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

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

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.

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.

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.

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

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

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.

@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

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.

will this be merged soon?

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

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

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

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.

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.

@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.

@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

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

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!).

🤖 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!