QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.71k stars 1.29k forks source link

creating an onGet in a layout and an index file collide with each other. #1553

Closed lifeiscontent closed 1 year ago

lifeiscontent commented 2 years ago

Qwik Version

0.9.0

Operating System (or Browser)

Chrome / MacOS

Node Version (if applicable)

16.13.2

Which component is affected?

Qwik City

Expected Behaviour

when specifying multiple nested onGet calls between layout.tsx and index.tsx files I'd expect them all to resolve with their specified resource data.

Actual Behaviour

when specifying multiple nested onGet calls between layout.tsx and index.tsx files they seem to all resolve to the leaf layout/index that had an onGet defined.

Additional Information

if this is as designed, that's fine, but I'm not sure where in the qwik city docs it define this behavior.

this is the page I ran into this issue on: https://github.com/lifeiscontent/qwik-realworld/blob/main/src/routes/index.tsx and layout: https://github.com/lifeiscontent/qwik-realworld/blob/main/src/routes/layout.tsx

lifeiscontent commented 2 years ago

Also, if it's helpful, this app isn't finished by any means, but this is the starting point of how I ran into this issue, any feedback on how you would do things differently would be greatly appreciated, happy to also add this as a Qwik example on the site! https://github.com/lifeiscontent/qwik-realworld

petebacondarwin commented 1 year ago

It would be a nice mental model if each onGet() in the chain of nested layouts would be called. Although I think that might require a change to the useEndpoint() API so that you could specify which onGet()'s data you want to access. Perhaps this could be modelled as a kind of context? Or perhaps onGet() could call useContextProvider() to attach data to a well known context object? And then rather than useEndpoint() one could just useContext()?

lifeiscontent commented 1 year ago

@petebacondarwin what is the current recommendation for fetching data in a larger application built on qwik / qwik city?

petebacondarwin commented 1 year ago

If you are using QwikCity to create an MPA (i.e. no client-side navigation) then I think simply putting an onGet() handler in each page is a reasonable approach. That way you can control closely what data is retrieved for the specific page being displayed. It gets more complicated if you start using client-side navigation since the old page template can currently be executed again with the new page data. You might have more power if you make use useResource() hooks rather than relying upon onGet() handlers. I don't yet think we have nailed down what the best architectural pattern is for larger applications.

lifeiscontent commented 1 year ago

not sure if it's helpful, but imo https://remix.run/ has one of the best experiences for dealing with data fetching/mutation. might be worth copying/taking inspiration from.

ramsaylanier commented 1 year ago

Im also running into this issue. I have a main layout.jsx that fetches user posts to display in a sidebar using onGet, and then a pages/[slug] route that fetches individual page data using onGet. When I'm at the page/[slug] route, the main layout route returns data from the lowest onGet. I tried using useResource without any luck, but perhaps I was implementing it wrong.

EDIT: I've figured it out by using useResource instead of onGet, though I'd love to rely on onGet as its only called from the server whereas useResource is called both places, ya?

qdelettre commented 1 year ago

Hi, i'm playing with qwik/qwik-city, it seems i'm trying to use onGet with the same structure as you @ramsaylanier

On local it seems to work, but once deployed it does not : see this PR and the last deploy on netlify

shairez commented 1 year ago

thanks for the report @lifeiscontent is this still the case with the latest version?

lifeiscontent commented 1 year ago

@shairez not sure, I haven't played with Qwik again since I opened the issue.

shairez commented 1 year ago

thanks @lifeiscontent !

Looks like the new server actions should solve this issue will wait for Manu to closed this one after he's done