🐛 BUG: `fetch` is unavailable until it isn’t

jonathantneal commented 3 years ago

What package manager are you using?

npm

What operating system are you using?

Mac

Describe the Bug

Within the header of an .astro file, external .js files cannot usefetch().

Example

/src/pages/index.astro:

---
import { fetched_metadata } from '../fetched.js'

const metadata = await fetched_metadata
---
<html lang="en">

<head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width" />
    <title>{metadata.siteTitle}</title>
</head>

<body>
    <h1>{metadata.siteTitle}</h1>
</body>

</html>

/src/fetched.js:

const fetched_metadata = fetch('http://localhost:3000/metadata.json').then(
    response => response.json()
)

export { fetched_metadata }

/public/metadata.json:

{
  "siteTitle": "Astro"
}

However, if I move the const fetched_metadata logic out of /src/fetched.js and into /src/pages/index.astro it will work.

And then, if I undo that change, it will keep working!

It’s as if using fetch is not allowed in external .js files, until it gets used in an .astro file, at which point it gets put into the global scope and becomes available to the .js files or something.

Steps to Reproduce

Install astro, or create an astro project using npm init astro.
Follow the steps outlined above.
See the errors described above, and the work-around described above.

Link to Minimal Reproducible Example (Optional)

https://github.com/jonathantneal/astro-fetch-outside-astro-issue

drwpow commented 3 years ago

This is definitely a bug. Should be as simple as injecting isomorphic-fetch into all .js files as we do .astro files (example).

itday commented 3 years ago

There is an article in the Astro documentation: Data Fetching#using-fetch-outside-of-astro-components.

Add import fetch from 'node-fetch'

matthewp commented 3 years ago

If anyone wants to work on this, it's probably just a matter of adding code to the transform step here: https://github.com/snowpackjs/astro/blob/d0e7fcfc0652c8e5b6af0b584838b605efb92a8a/packages/astro/snowpack-plugin.cjs#L27

If the file is .js or .ts add the fetch import. This will make it available in all JS files.

matthewp commented 3 years ago

Actually, the above is not a good solution, this will also affect frontend code!

Probably the simple way is better and just add globalThis.fetch = fetch in the runtime.

johnhenry commented 3 years ago

Should the fix be: a) Ensure that "fetch" is consistently present in all build-time .js files OR b) Prevent "fetch" ever being automatically present, forcing the user to use "node-fetch" in these situations.

I believe that a) would be fun and convenient, but b) -- having to import fetch -- wold be consistent with the rest of the node. As @itday mentions above, this already seems expected in the documentation.

(Tangentially, deno has built-in 'fetch'. In this case, a) would be consistent. I wonder if there's been any thought toward selecting build-time environment: e.g. astro build --buildtime=deno.)

matthewp commented 3 years ago

@johnhenry Global fetch was already accepted via the RFC process here: https://github.com/snowpackjs/astro/issues/410 . So this is a bug.

The solution here is to make fetch available on the globalThis object, rather than trying to make it an import like the current implementation. This will ensure that it's always available in all files.

matthewp commented 3 years ago

Here's a quick solution: https://github.com/snowpackjs/astro/commit/f22a1cc7b8c75dffce8ae6cdaff531d1f1b4d529

natemoo-re commented 3 years ago

Fixed in #1406!

withastro / astro