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 hello@remix.run.

Thanks!

- The Remix team

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

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

🦋 Changeset detected

Latest commit: 29181b1

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

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

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?

@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

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

@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

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

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.

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

@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"] }?

is this still an issue??

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

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

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