fix(remix-dev): set esbuild's serverPlatform to browser for Cloudflare & Deno

[rgdelato]

Setting esbuild platform to "browser" for Cloudflare and Deno to fix errors with using various libraries on those platforms.

Closes: #3120, algolia/react-instantsearch#3547

For additional context, this setting was originally set to neutral in #1504

• Tests

Testing Strategy:
I'm having some difficulty writing a test for this. I've been able to locally run two playground apps that are fixed by this change (one that imports react-markdown and one that imports algoliasearch), but I would appreciate some help with writing an integration test!

[remix-cla-bot]

Hi @rgdelato,

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 CLA Signed label will be added to the pull request.

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

[remix-cla-bot]

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

[RyKilleen]

Looking forward to this landing! Just ran into it in the context of trying to use Sanity in the workers template

[changeset-bot]

🦋 Changeset detected

Latest commit: 29181b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages

Name	Type
remix	Patch
@remix-run/dev	Patch
create-remix	Patch
@remix-run/css-bundle	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/deno	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/testing	Patch
@remix-run/vercel	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[schpet]

is it possible to use these changes before this is merged? i.e. by adding something to remix.config.js? or do i need to run a fork of remix until this is merged?

[machour]

@schpet There is no configuration in remix.config.js that would use this PR, but you can use something like https://www.npmjs.com/package/patch-package to achieve what you want

[schpet]

@machour thanks so much for this info!

with this patch i was able to successfully get react-markdown working on remix deployed to cloudflare pages (and locally via wrangler)

[apalumbo]

@MichaelDeBoey sorry for tagging you directly, from the source code it seems that you are the last one that worked on the file, it would be great if this fix can be integrated . Thank you

[acoreyj]

I'm honestly a bit concerned this has been an issue for so long, is there a better way to flag important community pull requests to the Remix team?

Note that this blocks being able to use @authjs in remix cloudflare

nextauthjs/next-auth#6270

[MichaelDeBoey]

I'm honestly not sure if we should support "browser" as value to serverPlatform as the ESbuild platform docs themselves are saying we should set it to "node" when we're generating code for the (Node) server.

If your bundled code is intended to run in node instead, you should set the platform to node

@rgdelato @RyKilleen @schpet @apalumbo @acoreyj I think the solution for #3120, algolia/react-instantsearch#3547, nextauthjs/next-auth#6270, Sanity, ... would be to wrap your components in something like @sergiodxa's remix-utils' ClientOnly component.

[acoreyj]

set it to "node" when we're generating code for the (Node) server.

Hey @MichaelDeBoey for more details I think the issue stems from how cloudflare workers use V8 but not node, they do implement a lot of web apis so it seems it's more similar to deno than node? I'm guessing that's why remix currently sets the esbuild platform to "neutral" instead of "node" for cloudflare (same as for deno).

The error that happens though is because the wrangler package for building to cloudflare uses a package "node-modules-polyfills" which when esbuild is in neutral can't seem to properly find the exports. So right now in my project if esbuild is "neutral" I get the error

✘ [ERROR] No matching export in "node-modules-polyfills:crypto" for import "createHmac"

    node_modules/@panva/hkdf/dist/node/esm/runtime/fallback.js:1:9:
      1 │ import { createHmac } from 'crypto';
        ╵          ~~~~~~~~~~

But if it is set to "browser" it works.

[MichaelDeBoey]

Tagging @evanw (the creator of ESbuild) to see if ESbuild's platform should support worker/deno as a value or if it's intended to use browser in these cases. 🤔

[rgdelato]

@MichaelDeBoey As far as I can tell, one of the main issues is that these packages specify various entry points for different platforms in their package.json (here's an example from react-dom, which uses both the exports and browser versions of specifying their exports (also here's their associated PR)).

My understanding is that Cloudflare wants to resolve "worker" or "browser" exports when they're specified. I think esbuild's conditions can cover the exports version and main fields can resolve the top-level browser version.

So maybe esbuild should support worker as a platform value, no idea there, but in the meantime, if we don't want to set platform to "browser" for Cloudflare/Deno, would it make sense to instead have a config like { mainFields: ["browser", "module" , "main"], conditions: ["worker", "browser", "module"] }?

[silence48]

is this still an issue??

[MichaelDeBoey]

Hi @rgdelato!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts.
I would also suggest to update the templates with the necessary changes as setting serverBuildTarget is now deprecated and will be removed in v2

[github-actions]

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

[rgdelato]

Apologies for forgetting about this, I was on an anniversary trip! But also, I wrote this PR like a year ago, and I would gladly defer to anyone else who has more familiarity with the codebase. (If not, I might try to take another look at this issue in a couple weeks.)