fix(remix-dev/compiler): fix Yarn PnP resolution for empty modules

[alisd23]

Closes: (No issue yet)

• Docs
• Tests

Testing Strategy:

Still no testing strategy for yarn pnp compatibility (though sounding like we might need to work out how to integration test this, given the complexity)

Related issues/PRs

This is another fix for yarn pnp, in the same realm as #3594, and related to this fix in @yarnpkg/esbuild-plugin-pnp by @jenseng, with the resolveDir esbuild property. However, this may not solve the issues with Deno that @lensbart is having.

This also un-does #3579 because that didn't actually work at all, just hid some errors (I was lazy there). After looking in much more detail I think I have solved a related problem.

Issue/fix

I've noticed that all imports in the pnp namespace being resolved (in the onResolve esbuild callback) have an empty resolveDir value by default, which is actually breaking the remix EmptyModulesPlugin for pnp imports. That plugin assumes a resolveDir is present, and uses it to find the absolute path of files in the app folder, which does not work correctly when resolveDir is empty.

I've created a fairly easy fix in remix land, but wondering if this should instead be fixed in @yarnpkg/esbuild-plugin-pnp. Though I'm not sure if this is the expected behaviour from that plugin (maybe @jenseng has a better idea?).

I've seen here that the resolveDir can be returned from the esbuild onLoad callback in the plugin, I guess by just finding the directory part of the import path.

Client bundle issue (very related)

@MichaelDeBoey I've also noticed a similar issue when using yarn pnp, where even with this fix, imports from @remix-run/node fail in the browser because that module tries to require node built-ins (fs, path, etc.), which then throws errors in the browser. I'm not sure how this works with regular node resolution because the EmptyModulesPlugin actually doesn't strip out @remix-run/node imports in the client bundles at all. Have you got any ideas about this works with normal node resolution? Is it being stripped out elsewhere or is it failing silently, whilst yarn fails very loudly when it sees those node imports?

My hacky fix at the moment is to just re-export all @remix-run/node exports from a file like node.server.ts, so the EmptyModulesPlugin picks it up and removes it from the bundle. But maybe remix-run/node itself should automatically be explicitly removed from client bundles too?

[jenseng]

Yeah this feels like a good change to make in @yarnpkg/esbuild-plugin-pnp ... although it's not strictly required to set a resolveDir in the result, it would make that plugin play nicely with other plugins and shouldn't have any negative consequences.

[alisd23]

Fix PR up in the yarn repo here: yarnpkg/berry#4597

Waiting for feedback, then hopefully merged fairly fast.

Regarding the issue I mentioned with @remix-run/node and the EmptyModulesPlugin, maybe I'll create a separate issue for that after this fix is sorted, as it might not be tightly related to this specific issue.

[alisd23]

The downstream fix to @yarnpkg/esbuild-plugin-pnp has already been merged, so this change now basically consists of:

• Upgrade @yarnpkg/esbuild-plugin-pnp to newest version
• Undo the change from fix(remix-dev): fix Yarn PnP resolve issue #3579 which was not correct

[MichaelDeBoey]

Let's wait for this to merge until @yarnpkg/esbuild-plugin-pnp v3 is released.

Putting this in draft in the meantime.

[alisd23]

@MichaelDeBoey do you mean wait for the next non-RC version to be released? I don't think that'll be any time soon.

I asked one of the yarn maintainers when this would happen, and they said only on the next major release of yarn (v4), which could be a while. Could we upgrade now so we can continue working towards pnp/remix compatibility?

I've been looking at the pnp/remix integration quite a bit recently and I don't think there's any other changes needed on the plugin side - just a couple things left to do in remix world.

[jenseng]

+1 on not waiting… this plugin’s release cycle/versioning is unfortunately linked to yarn, but that’s really a side effect of how their monorepo/releases are set up. IMO there aren’t any stability/support concerns in using this version, as the only changes since 2.0.2 are these two bug fixes. 2.0.2 is known to be problematic, so this is strictly better than the status quo.

…

On Sun, Jul 3, 2022 at 8:31 AM Ali Sheehan-Dare ***@***.***> wrote: @MichaelDeBoey <https://github.com/MichaelDeBoey> do you mean wait for the next non-RC version to be released? I don't think that'll be any time soon. I asked one of the yarn maintainers when this would happen <yarnpkg/berry#4597 (comment)>, and they said only on the next major release of yarn (v4), which could be a while. Could we upgrade now so we can continue working towards pnp/remix compatibility? I've been looking at the pnp/remix integration quite a bit recently and I don't think there's any other changes needed on the plugin side - just a couple things left to do in remix world. — Reply to this email directly, view it on GitHub <#3633 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2XMKZ7JQRBG67D47FF3TVSGP4VANCNFSM52NDHMNQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[alisd23]

@MichaelDeBoey Sorry to push, but I'd really like to get this merged if possible please? There's a couple of other pnp/remix issues that I'm currently working around with some hacks, but this is a complete blocker at the moment for me sadly

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-a555e0b-20220707 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!

[alisd23]

Thanks all!