Skip to content
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

Cloudflare not loading from lib/browser.js #15

Open
inerd89 opened this issue Feb 23, 2022 · 4 comments
Open

Cloudflare not loading from lib/browser.js #15

inerd89 opened this issue Feb 23, 2022 · 4 comments

Comments

@inerd89
Copy link

inerd89 commented Feb 23, 2022

Love picosanity! However, I've been having an issue when trying to deploy to Cloudflare Workers. For some reason, Cloudflare isn't honoring the browser field in picosanity's package.json. It's trying to run from lib/index.js which won't work in CF's non-node environment. I am able to fix by deep importing:

import PicoSanity from 'picosanity/lib/browser'

This same problem has been happening in other data source clients as well. The solution proposed in this pull request for Fauna also fixes the issue with picosanity for me and, as mentioned in the PR, may be a better solution than deep importing when using Typescript:

/* add to package.json */
"exports": {
  "require": "./lib/index.js",
  "default": "./lib/browser.js"
},

Bringing this to your attention as a possible fix to avoid having to do a deep import.

(This problem also exists for @sanity/client.)

@rexxars
Copy link
Owner

rexxars commented Feb 23, 2022

I cannot replicate this - in which mode are you? Webpack? Service worker? ES Modules?

@inerd89
Copy link
Author

inerd89 commented Feb 23, 2022

Apologies—should have given more details!

I actually put together a test repo demonstrating the issue as I was trying to find a fix yesterday. I'm using Remix and its default CF template, which uses the service worker format. The deep import fix is demonstrated in the cf-workers-picosanity directory within that repo—this file is where the actual import is and there is a wrangler.toml you can reference in there as well.

A bit more background: I ran into the issue while trying to set up a Sanity.io + Remix + CF Workers stack. I was able to connect to Sanity using either @sanity/client or picosanity and pull data via Remix loaders when I was using the local Remix App Server as my deploy target. When I generated a Remix app with Cloudflare Workers as my target and copied in the /app directory from my working app (per the remix docs), I got an error when firing up the dev server: TypeError: globalThis.XMLHttpRequest is not a constructor. Others in the Remix discord had similar issues with other data source clients and we eventually realized it stemmed from the non-browser versions of packages being loaded in CF.

My test app is based on this guide on sanity.io. The example app in the guide doesn't deploy to CF, only the local app server, so the issue never came up.

This could be a Remix issue—perhaps it is their bundler setup that is resolving to the wrong (non-browser) file?

@rexxars
Copy link
Owner

rexxars commented Feb 23, 2022

It seems like the remix compiler explicitly says to use the module or main field, not allowing browser. I'm honestly not sure what the best practice is here - we want anyone that is bundling picosanity into a webpack bundle to use the browser version, eg the one that uses the native fetch, while anyone using node should get the version that imports node-fetch.

If I understand how the exports require/default works, this wouldn't actually work in our case - it would just use the browser version for anything that uses imports (eg non-require), which is the case for both node and the browser, in many cases.

I'm not sure quite what the right behavior for main fields should be in terms of cloudflare workers - but I guess it kind of leans more towards a browser-like environment than it does a node environment. Knowing that, I feel like perhaps remix not respecting the browser field might be the culprit here?

Thoughts?

@inerd89
Copy link
Author

inerd89 commented Feb 25, 2022

You make some great points. As a node user who uses import rather than require() myself, it does seem that the exports field isn't the best solution. Really, this feels like a solved problem with the browser field, at least when it is respected.

So, I think you are right—it does seem like remix not respecting browser is the culprit. Perhaps we wait and see if they fix their compiler? Thanks for hearing me out on this, in any case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants