🐛 BUG: Problems with isomorphic packages

waspeer commented 3 years ago

Bug Report: Quick Checklist

[x] I am on the latest version of Astro & all plugins: v0.13
[x] I am on an ESM Node.js version: v14.15.3
[x] I use package manager: Yarn
[x] I run Astro on OS: Mac

Describe the bug

When trying to fetch data with @sanity/client I got the error window is not defined. It looks to me like Astro is trying to execute code that is bundled for the browser by Snowpack. With this package specifically it seems to be in issue for two of its dependencies: @sanity/eventsource and get-it. Both of these packages seem to have to some logic that makes them isomorphic, but cause issues in this case.

Steps to reproduce

yarn create astro using template Starter Kit (Generic)
Install @sanity/client
Import it somewhere
Run yarn start
Get error ReferenceError: window is not defined

I have a repo with two commits that follow these steps for you: https://github.com/waspeer/astro-sanity-bug-example

Expected behavior

I expected all the code written or imported into the 'frontmatter' of a page to be compiled for Node, since it does not ship to the browser.

khrome83 commented 3 years ago

Does this change if it import this somewhere else, more specifically -

If you had an pages/index.astro and it imported <MyComponet:idle /> and MyComponent is what imported @sanity/client would this work? The idea being that index.astro is built during export, but it knows that MyComponent is required later in the chain and partial hydrated when the window is present.

matthewp commented 3 years ago

At the moment there's not a great way to have code that runs only in the client. Anything you import in an Astro file (or a component for that matter) is part of the graph on the server. We've talked about adding a :client but even that would not prevent the graph from executing on the server.

If you need something to truly only run on the client, you can use a module script tag which will be processed by snowpack. Your project is available under the /_astro/ URL, so for example if you have something in src you can do:

<script type="module" src="/_astro/src/client/SanityThing.js"></script>

And it can do whatever it needs to do.

waspeer commented 3 years ago

I realise I was not very clear in my issue about what I'm trying to accomplish. My goal is not to have this package to run client side. The sanity package is a data fetching package that is built to work on the client as well as on the server, which is why this specific error is surprising. My suspicion is that is has to do with the settings in Snowpack, but I'm not familiar enough with it to be sure.

iksaku commented 3 years ago

Just had the same issue with a project of mine. Instead of dealing with sanity, I'm dealing with @octokit/rest To not repeat my self a lot, here's the full description I sent over on discord.

Since components are being compiled on the server, the shipped octokit package is trying to pickup node-fetch.

@natemoo-re made an interesting suggestion, which is to replace node-fetch with isomorphic-unfetch in Astro's src, however, I'm not sure if there are further implications with making this switch.

FredKSchott commented 3 years ago

This is a Snowpack issue (technically, an esinstall issue) related to SSR:

Snowpack/esinstall always prefers the "module" and "browser" entrypoints when installing, over "main" and "node"
This is great in the browser
when we run this on the server, a package's "module"/"browser" entrypoint is still being preferred
as a result, the wrong package code runs that was specifically designed not to run on the server

@drwpow is going to kick off some work on Snowpack to address some of these issues. We can keep this issue around to track this on the Astro side, but unfortunately (unless I'm completely wrong on this) there's not much that we can do here in Astro to solve.

luchoster commented 3 years ago

having the same issue

I was wondering if there's a way to bypass the window error, kinda how Gatsby does (https://www.gatsbyjs.com/docs/debugging-html-builds/#fixing-third-party-modules)

otherwise, we can't use @sanity/client to fetch data?

astro-500

waspeer commented 3 years ago

I don't think that would work in this case. Gatsby's solution is to disable the module server-side, but that would also cause the data to not be fetched which is kinda the point. I guess a workaround could be to expose a global window object with just the fetch function on it, since I suspect that's whats being used 99% of the times.

YoungElPaso commented 3 years ago

I'm having a similar issue but with Svelte - I'm trying to fetch some data in a Svelte component, but get an error w/ fetch because of SSR, but conversely can't import node-fetch or cross-fetch to handle that isomorphically... This is a bit of a conundrum, or perhaps really a dilemna.

I suppose its theoretically possible to share state across 'islands' and so load the data via an .astro file (where I've read fetch is supported?) but that seems less than ideal.

fev4 commented 3 years ago

I'm having the same issue. Had to use the native fetch in order to avoid this error. Not sure what other problems are carried over by not using the Sanity client (as I'm new to it), but so far that's the quickest fix.

jasikpark commented 3 years ago

https://github.com/snowpackjs/astro/pull/949 will hopefully help with the "using fetch" problem, but it probably doesn't help with the general problem..

natemoo-re commented 3 years ago

There have been quite a few updates since this was last commented on!

astro@next is now backed by Vite rather than Snowpack. This fixes a lot of issues with SSR packages. There might still may be some packages that have to be manually configured to not run during SSR, but I think this particular issue will definitely be closed.
node-fetch will now injected only during SSR, so it should make its way into the client. You won't have to use node-fetch or cross-fetch anywhere, it should just work (even in Svelte components)
The client:only directive has been added to handle components that should only be rendered on the client. This is a good option for use cases where third-party components access DOM objects like document or window unconditionally.

natemoo-re commented 3 years ago

Following up on my previous comment, while there are bound to be some differences between Snowpack and Vite for SSR, this issue should be considered addressed. The team has decided to close out issues that have been confirmed as fixed by astro@0.21.0-next.0, astro@0.21.0-next.1, or astro@0.21.0-next.2. Our hope is that this will help the v0.21 milestone remain as actionable as possible.

To verify that this issue has been fixed, you may

Open a new astro@next project on StackBlitz

Locally upgrade to the astro@next preview

npm i astro@next
# If using Framework renderers...
npm i @astrojs/renderer-name@next

If you run into other SSR problems with astro@next, please open a new issue.

withastro / astro