sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
17.88k stars 1.8k forks source link

New routing DX can still be improved re REST, there should be one default routing which does it all (with repo) #7030

Closed newsve closed 1 year ago

newsve commented 1 year ago

Describe the problem

Having many REST routing options is great but can be also overwhelming at the same time. There should be one default option which does it all.

Before continuing reading, please check following repo's README's comparison table for all the options including code: https://github.com/newsve/sveltekit-routing-options

There, you see that "Comfy" should be the default option because it's minimal, offers SSR and preserves types. Only if you need your endpoints to work as dedicated JSON ones you should consider "Flexy" and if you need max. one serialization run paired with special types, you should consider "Basic". But again, in an ideal world all users, in particular new ones, shouldn't bother with any other than "Comfy" and just forget that the other options exist.

Describe the proposed solution

"Comfy" would be feature-complete if its +page.server.ts could also work as ded. JSON endpoint. Having only one serialization run would be a nice-to-have but isn't crucial since devalue is fast. But again, offering latter option too would make all other options obsolete and improve the general DX. Then, we wouldn't even need to document the other options anymore making onboarding much easier.

Alternatives considered

none yet

Importance

would make my life easier

Additional Information

No response

dummdidumm commented 1 year ago

I'm not really sure what the feature request or proposed solution is - could you clarify? As you have shown, all these things are achievable using existing features, and you can mix and match them if you like. Are you proposing to remove certain features?

newsve commented 1 year ago

what the feature request or proposed solution is - could you clarify?

Apologies for being not clear! I try again:

  1. Make +page.server.ts feature-complete
  2. (optional) Make +page.server.ts the default way to route in the docs
  3. (optional) Declare other options as exceptions/fallback, give them less visibility/air time in the docs/tutorials

Re 1:

How to hack a +page.server.ts endpoint to be a stand-alone pure JSON endpoint (will this stay?):

<script lang="ts">
  async function getKeyboard() {
    const res = await fetch('/comfy/__data.js');
    const resAsText = await res.text();
    (0, eval)('(' + resAsText + ')');

    // @ts-ignore
    return window.__sveltekit_data.nodes[1].data;
  }
</script>
dummdidumm commented 1 year ago

I see. Regarding 2./3., this is worth discussing what to show where/when in the docs. This also plays into the tutorial which we need to make a concept for first. Regarding 1: You can place your +server.ts next to your +page.server.ts if you need JSON endpoints. The hack is nothing we want to bless and something that should not be relied upon. If you need to update data that should be done through invalidate.

Closing as there's nothing actionable from this, but rest assured we'll take 2./3. into account when thinking about the docs and the tutorial.

newsve commented 1 year ago

You mean like this?

// +server.ts
export async function GET() {
    const keyboard = {
        name: 'Huntsman Mini',
        layout: '60%'
    };

    return new Response(JSON.stringify(keyboard));
}
// +page.server.ts
import { GET } from './+server';

export async function load() {
    const res = await GET();
    return await res.json();
}
dummdidumm commented 1 year ago

Yes that would be one possibility. load does also return the data of its parents, which in this situation probably isn't what you want. If you want that, then you should do this instead:

<script>
  import { invalidateAll } from '$app/navigation';

  export let data;
  function updateAllData() {
    invalidateAll();
  }
</script>
newsve commented 1 year ago

load does also return the data of its parents

just checked the docs but would you mind to give an example for "data of its parents", thanks!

dummdidumm commented 1 year ago

If you have a layout, that data is part of the returned, data, too. export let data: { fooFromLayout: .., barFromPage: .. }

newsve commented 1 year ago

Thx! Again, re "place your +server.ts next to your +page.server.ts": This doesn't preserve types like a lonely +page.server.tsendpoint. Manual re-declaration and exporting of inferred response types isn't always possible or cumbersome since they are within a function.

"Smaller" cons (but bearable): more code/boilerplate, unnecessary stringify/.json run but again they are small and ok. Not having types is quite a bummer when using such an endpoint with its own +page.svelte (so not as a stand-alone endpoint where I wouldn't expect types).

Hope I could make this con clear.

Edit: I know that this is nuanced issue but enough to confuse many.

dummdidumm commented 1 year ago

Inferred types are not possible with +server.ts because it returns a Response object, that can be anything, not just JSON. They also can be called from any location, not just on the +page.svelte next to it. In general +server.js provides more flexibility, is less opinionated, at the cost of losing the type inference goodies and a little more code.

newsve commented 1 year ago

Thanks again. So, after a bit more testing, I find following solution the most comfy, versatile and fool-proof:

1. Always use +page.server.ts as only endpoint (no +page.ts, no +server.ts) 2. If you need 1 to be a standalone endpoint use following fetch wrapper in +page.svelte:

export default async function (resource: string, queryString: string) {
  const res = await fetch(`${resource}/__data.js${queryString}`)
  ;(0, eval)('(' + (await res.text()) + ')')

  // @ts-ignore
  return window.__sveltekit_data.nodes[1].data
}

3. Have a test that checks if the undocumented API in 2 is still working; if not patch (important when updating SvelteKit)

This gives me (almost) all features:
✅ SSR
✅ Types (when used as shadow endpoint) ✅ Standalone endpoint ✅ Zero boilerplate ✅ Only one file ✅ Consistent style over all endpoints and no mental drain for the team ("which endpoint style should I take/was taken...") or need to educate them ❌ no redundant de-/serialization run of devalue

Is it elegant? Yes, except point 3: Tracking on every update if __data.js endpoints are still working is ugly but far better than all the other options with their drawbacks.

I'd be happy if you guys looked again into this issue and maybe you have even better ideas. 🙂

Looking forward to your feedback!

david-plugge commented 1 year ago

I don´t get the point tbh. If you consume the data in your app, use +page[.server], if not use +server.

newsve commented 1 year ago

You need also a standalone endpoint in your app if you want to refetch some data after the client is loaded already. So, sometimes you need both, it's not one versus the other. Is this what you don't get? Hope I got you right.

dummdidumm commented 1 year ago

If you want to get data after the client is loaded already and that data is new, you should use invalidate instead, and call it in a way that makes the load function rerun.

newsve commented 1 year ago

That's a good idea but what if the fetch's query string changed for the refetch?

dummdidumm commented 1 year ago

That works.

export async function load({url, fetch}) {
  const response = await fetch(url); // <- reruns when the URL changes, this includes the query string
  return response.json();
}
newsve commented 1 year ago

Thanks! How do I trigger a refetch then whenever I like? With invalidate? If yes, how do I pass the new query string then? Your last snippet is from +page.ts, right?

dummdidumm commented 1 year ago

In this case you don't need to call invalidate on the client, as long as you change the URL in the browser it will work as expected.

newsve commented 1 year ago

Ok, but what if I don't want an automatic refetch on history.replaceState() but only when I explicitly fetch()

Use case: you change the URL with history.replaceState() for sharing purposes but don't want to refetch then every time. This might sound like a niche use case but it isn't.

dummdidumm commented 1 year ago

There's a pending feature request, which would introduce a way to ignore certain accesses to the load parameters, which you would need for that. Then you could do it manually by opting out of the automatic load invalidation, and opt in by adding manual dependencies which you invalidate:

export function load({ url, fetch, ignore, depends }) {
  const result = await ignore(() => fetch(url)); // ignore doesn't exist yet, this is just an example
  depends('reload:me');
  return result.json();
}
<script>
  import { invalidate } from '$app/navigation';
  export let data;
  // ..
  function refetch() {
    invalidate('reload:me');
  }
</script>
..
newsve commented 1 year ago

@dummdidumm sounds exciting and I do look fwd to it! However, there are more use cases which won't get covered and worse lead to a suboptimal architecture, one more example:

This would only work with my hacked fetch wrapper. If I use +server.ts instead of +page.server.ts I'd face:

Not sure if I could create a better understanding. It's just that SvelteKit restrict users for a pretty important use case and I'd be happy if you guys looked into this once again. Thanks! 🙂

dummdidumm commented 1 year ago

For this use case, create a +layout.server.ts file inside keyboard - the data from it will be available to all sub pages, too.

newsve commented 1 year ago

Then, how would you control/restrict that each detail page gets only their related keyboards (e.g., Logitech doesn't get Razer keyboards) and you don't get data you don't need?

And, how can the page /mice/[mouse_id] pull complementing keyboards?

dummdidumm commented 1 year ago

That's contradicting. Above you said you want to reuse the logic. Now you say the logic is different on the sub page. If the logic is different, then you need to have different load functions anyway, at which point there's no harm in moving the logic what to load when out into something common.

newsve commented 1 year ago

Sorry, I wasn't clear enough: The /keybaords JSON endpoint can be adjusted/filtered through a search string. So, it's the same endpoint through all given examples: fetch('/keyboards') on the keyboards overview page, fetch('/keyboards?brand=12') for related Razer keyboards on the Huntsman Mini detail page, fetch('/keyboards?brand=13') for Logitech on the detail page of the MX Keys (a Logitech keyboard) and fetch('/keyboards?category=gaming') on the mouse detail page of the Razer Viper Ultimate (some mouse). In last three examples, it's the endpoint only for the related products, of course not for the main product from which details are shown.

Edit: Otherwise the frontend team would need to ask the backend team for a new endpoint every time they need a new filter. This would not just slow down both teams and overall pace but also increase API complexity (re discovery and maintenance) very quickly.

dummdidumm commented 1 year ago

If they all use the same endpoint, then create one unified endpoint in some +server.js file and all load functions load from there.

newsve commented 1 year ago

Then, I don't have types preserved in /keyboards.svelte, three serialization runs and the other aforementioned stuff.

Out of curiosity: May I ask what drawbacks we would face if the current ...__data.js endpoint was made official? I like it quite a lot. 🙂

dummdidumm commented 1 year ago

I still don't understand the use case fully, but I would rather write a few more lines of types/code and use the official APIs. I would discourage you from using __data.js, it's an internal mechanism, not part of the public API, so use at your own risk.

newsve commented 1 year ago

Sorry, but I wasn't clear again. The more boilerplate is the smallest problem (still not nice). it's lacking types and that you have to work-around around the framework.

And, yes I understood that __data.json is internal, the question was a different one though: Why do you want to keep it internal if there if there're enough valid use cases with no as intuitive options?

dummdidumm commented 1 year ago

To me it's still not clear why using __data.json would be better in your situation. I guess I need to see some actual code to better understand this. Lacking types isn't a problem, you can add them yourself. You don't get type support either if you use fetch on the client like shown in your examples.

newsve commented 1 year ago

Happy to come full circle and go through all bullets another time. However, before we do this, would you mind to give drawbacks of making the internal API __data.js official? Thanks.

dummdidumm commented 1 year ago
newsve commented 1 year ago

Ok understood. Since ignore certain access isn't there yet, I probably found a better idea: load in +page.server.ts is the default. To make it "fetchable" from anywhere we'd just need to add this +server.ts which is more of a redirect:

import { load } from './+page.server'

export const GET = async (requestEvent: any) =>
  new Response(JSON.stringify(await load(requestEvent)))

=>

Still need to get the types resolved, any is not right but if I declare it as RequestHandler properly I'd get Argument of type 'RequestHandler' is not assignable to parameter of type'ServerLoadEvent<RouteParams, {}>'. (tsserver 2345) or I just leave it any, it's stringified afterwards anyway, let's see.

What do you think?

david-plugge commented 1 year ago

@newsve interesting! You can create a helper that also fills in missing properties

function requestEventToServerLoadEvent(event: RequestEvent) {
    const ev = event as ServerLoadEvent;
    ev.depends = () => {
        /**/
    };
    ev.parent = async () => ({});

    return ev;
}

export const GET: RequestHandler = async (event) => {
    return json(await load(requestEventToServerLoadEvent(event)));
};

Edit:

Using parent() inside the load probably breaks the functionality so you could throw an error instead of returning an empty object

dummdidumm commented 1 year ago

I still don't understand why you don't do it the other way around; using the GET inside the load

newsve commented 1 year ago

@dummdidumm if you imported GET inside load (so +server.ts is the default and has all the code), you face:

@david-plugge thanks, what could be missing properties be IRL? (just to get some use cases)

dummdidumm commented 1 year ago

Then in this case I would move out the logic into some function which you can call as appropriate in both load and GET

newsve commented 1 year ago

Simon, another thing in this context: I just checked out the new forms api and there we can invoke the action from other pages which resides in +page.server.ts.

I don't want to reopen our prior convo and just asking out of curiosity but isn't this is an inconsistent API design? With form actions I can use other +page.server.ts' while I can't do this if it's about GET requests. Is this intended or do I miss something? Looking forward to your feedback!

dummdidumm commented 1 year ago

One thing is public API with a well-formed contract and one thing is internal API which is super weird to use when you try to it yourself, that's the difference.

newsve commented 1 year ago

er, I rather meant the general design decision to allow +page.server.ts accept json GET requests from the same path only while letting it answer POST requests from any other path. Hope I've been more clear.

newsve commented 1 year ago

Or: Wouldn't it be more consistent/intuitive to put form actions into +server.ts (which can serve any path per se)?

dummdidumm commented 1 year ago

No, because actions are bound to a page, and +server can exist without a page next to it

newsve commented 1 year ago

actions are bound to a page

Are they? They can be invoked from other pages as you guys stated in the spec.

Also IRL, POST paths are very often page-independent, i.e. a login component/modal is typically available on all pages but POSTing to the same json endpoint such as /sessions.

Don't want to bikeshed and after our conversation I won't forget all the nuances/inconsistencies of +server.ts and +page.server.ts but do you think it's easy/intuitive to grasp for newcomers?

dummdidumm commented 1 year ago

Actions are bound to a page because they can't exist without one. Calling them from other pages doesn't change that. load is different because all load functions leading up to a page make up the data for that page, not just the last load in the chain. Reloading that data is supported without navigation through invalidate.