Shopify / hydrogen-v1

React-based framework for building dynamic, Shopify-powered custom storefronts.
https://shopify.github.io/hydrogen-v1/
MIT License
3.75k stars 326 forks source link

Replace `useQuery` with a synchronous-looking version of `fetch` #256

Closed jplhomer closed 2 years ago

jplhomer commented 2 years ago

tl;dr

We should drop useQuery and introduce a simple fetch from the @shopify/hydrogen package which is a wrapper around react-fetch (or an intermediate implementation until we're able to use real react-fetch).

This implementation should accept Hydrogen-specific cache params:

const data = await fetch('https://my.api.com/data.json', {
  headers: {
    'content-type': 'application/json',
  },
  cache: {
    maxAge: 60,
    ...
  }
}).json();

Notes:

Background

React I/O libraries in a RSC world are trending toward a synchronous-looking pattern:

const data = fetch('https://my.api.com/data.json').json()

This isn't actually synchronous; it uses Promises under the hood. But it looks and feels like a synchronous JS function.

What the HELL, Josh?! I thought we just moved away from this synchronous style of JavaScript to a Promise-based async/await pattern?!!?!?!?!

Yeah I feel you. But stay with me...

I'm still learning about this idea, but by adopting this pattern, React gets superhero abilities to analyze and optimize data fetching within our apps.

Today, React throws a Suspense promise until the data resolves.

But in the future, it might be able to pause the execution of the thread, batch multiple requests together DataLoader style, and other super cool stuff.

But I just learned PROMISES. Can't I keep using them?

Promises and async/await are unforgiving. If you want to await a function, the parent scope has to be async. This trickles upward and downward.

Promises are "contagious," so we should avoid permitting user code to contain Promise or async/await patterns and lean heavily into React I/O libraries like react-fetch, react-pg, react-fs, etc.

This is why we should move away from useQuery which promotes Promise-based usage.

blittle commented 2 years ago

I'd add the usage of promises everywhere also has implications for error handling. The top level promise/async function needs to make sure to handle the error scenario. It is super easy to forget that.

In general, I support this. One of the supposed benefits of suspense is the synchronous fetching API. I think we should try and take advantage of it where possible.

frandiox commented 2 years ago

What the HELL, Josh?! I thought we just moved away from this synchronous style of JavaScript to a Promise-based async/await pattern?!!?!?!?!

This ๐Ÿ˜‚ -- But I guess the end justifies the means in this case for React ecosystem, so let's try the synchronous approach.

patrick-mcdougle commented 2 years ago

But isn't node still single threaded and requires yields to the event loop to continue execution of other tasks? By using async / await / promises you're yielding the thread to other requests / tasks that need the cpu while your network request sluggishly resolves DNS, establishes TLS connections, sends data over long wires, etc. Has something fundamentally changed in how node works?

jplhomer commented 2 years ago

@patrick-mcdougle AFAIK yes, today that is true. And since the current implementation of React's I/O libraries are backed by Promises yielding to other requests, it works as-is.

Other folks smarter than me have hinted at new JS runtimes that might support synchronous APIs in a different way.

See this comment from Sebastian.

patrick-mcdougle commented 2 years ago

I'd be hesitant to go all-in on moving to something that would hurt performance using today's technology for the prospect of some future runtime that might never exist.

jplhomer commented 2 years ago

I'd be hesitant to go all-in on moving to something that would hurt performance using today's technology

But it's not something that would hurt performance โ€”ย React's I/O libraries are still backed by Promises today, which as you mentioned yield the thread to other requests ๐Ÿ‘

patrick-mcdougle commented 2 years ago

Not following how something can be synchronous AND yield to the event loop. Those feel mutually exclusive to me. I guess I'm missing something fundamental.

jplhomer commented 2 years ago

@patrick-mcdougle whoops - the title of this post should have been titled "synchronous-looking version." I've updated it to prevent confusion! We won't be using a truly synchronous API until we have a runtime that uses it effectively.

jplhomer commented 2 years ago

Given that there is still an active exploration into how data fetching & serialization will work upstream in RSC, I propose we instead add fetch as an option and use it as the green path in our examples and starter template.

We can still offer useQuery as a way to run bespoke promises or non-HTTP things in a Suspense-friendly way, with the intention of deprecating it if/when we determine allowing code to be written with useQuery is detrimental.