-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(remix-dev): yarn pnp compatibility plugin #3594
Conversation
Hi @lensbart, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
const yarnPnpCompatibilityPlugin = NodeResolvePlugin({ | ||
extensions: [".ts", ".js"], | ||
onResolved: (resolved) => | ||
resolved.includes("node_modules") ? { external: true } : resolved, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the reasoning behind creating our own plugin instead of using the official one by the Yarn team please?
If changes to the plugin are needed, why don't we create a PR upstream for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an instantiation with default config according to the @esbuild-plugins/node-resolve
docs because it’s being used in two places in this file.
I’ve tried using both the previous plugin and this one, and this is the only one that works for me, hence the change.
I haven’t looked into what to fix in the other plugin in order to get it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this works, but I rather would keep using the official Yarn PnP plugin & create a fix upstream instead of running with our own version (and the burden of maintaining that ourselves) tbh.
Could you please create a compact reproduction repo & file an issue upstream?
That way we can first look on what's the problem on their end & maybe we can fix it in our repo with just a dependency update & without any extra maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll take a look over the weekend. (Although I still don’t quite understand how this increases our maintenance burden — the plugin is maintained here and is designed specifically for PnP compatibility.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big difference, but now we just import the official Yarn PnP plugin and that's it.
With your solution, we have to know the API (& options) of the @esbuild-plugins/node-resolve
plugin.
If they ever have a breaking change in there, it adds extra maintenance work on our side.
An API-less plugin won't have any impact then.
f014b1f
to
36ff8a4
Compare
@lensbart Out of interest, a couple of questions, to see if we can work out why the results are different for us.
I'll also aim to try the plugin you're suggesting in this PR, to see what results I get. |
@alisd23 I think my issues are mainly with choosing
Reproduction: run
cd my-remix-app
yarn set version stable
touch yarn.lock
echo "nodeLinker: pnp" > .yarnrc.yml
yarn I’d say an empty Using this setup,
This problem gets resolved by swapping out the Yarn plugins. |
I haven't looked into the specifics of what might be happening with Deno here, but I did fix another bug in esbuild-plugin-pnp around how it resolves relative paths (as part of getting #2027 passing again 😅 ), and the fix here might be similarly straightforward. If you do go that route, here are a couple pointers:
|
OK Interesting, that is a little more complex than my set up. Also I've spent a bit more time actually (many hours) playing around with my Remix app with these fixes, and actually neither my original quick fix (re-ordering), or the changes in this PR have fixed the issue for me. Yarn PNP seems like a bit of a minefield at the moment. Will try to look into more detail soon. |
Thanks for the reminder. I think not! |
Swaps out
@yarnpkg/esbuild-plugin-pnp
(which did not work nicely with@esbuild-plugins/node-modules-polyfill
) for@esbuild-plugins/node-resolve
(which is from the same repo as the latter).Also silences a harmless error when
yarn config get @remix-run:registry
throws because the setting can’t be foundfix(remix-dev): fix config call when using Yarn PnP #3566 intended to achieve the same effect, but was less effective, so I closed that PR.
fix(remix-dev): fix Yarn PnP resolve issue #3579 did not do the trick for me, and the plugin ordering seems not to matter with the new plugin, but it could be that @alisd23’s setup is different, so I left a comment about the plugin ordering just in case.
Probably should be tested by other people as well, given that previous fixes were brittle.
Interferes with fix(remix-dev): relativize route modules to make builds deterministic #2027, not sure which approach is preferable