jycouet / kitql

A set of standalone tools to SpeedRun WebApps!
https://kitql.dev
MIT License
401 stars 37 forks source link

vite-plugin-kit-routes: no route from only `+page.server.js#load` #655

Closed jangxyz closed 3 months ago

jangxyz commented 4 months ago

Describe the bug

It does not extract route if the route has only +page.server.js#load() function and no accompanying +page.svelte file.

I have an OAuth login route, that simply redirects to user to the external link. /login/github I can simply have <a href="/login/github">Login</a>, but wrapping the link with route() resolves to undefined. It seems the "page-server-only" route is not catched by any of PAGES, ACTIONS, SERVERS, and LINKS.

Severity

serious, but I can work around it

Steps to Reproduce the Bug

  1. Make a +page.server.js file with a load function, and only perform redirect.
  2. Do not make +page.svelte file.
  3. Now try to make a link to that route.

Reproduction

https://stackblitz.com/edit/github-bda3zz?file=src%2Froutes%2F%2Blayout.svelte

jycouet commented 4 months ago

Thx for reporting. Yes, load function doesn't create routes as usually it's to load data, not to redirect to a specific place.

What do you think about adding in your config something like:

kitRoutes({
  LINKS: {
    login: '/login/github',
  }
})

Let me know

jangxyz commented 4 months ago

I know it's not common to have a redirect-only route. I haven't particularly thought about declaring custom links for it.

While thinking about it, I guess it all comes down to being an uncommon case.

Declaring it in a separate config file sounds appropriate if I have plans to use it in multiple places, or if it may change in the future more frequently than I planned. Otherwise, it feels a bit much. For a one time usage, I feel like I can live without using the route() function at all.

Which, in my opinion, is why the library SHOULD catch it for me 😁 . Situation like this makes developer easily go out of the safe path, since it seems so subtle and there doesn't seem to be anything that may go wrong. It's just this one small case, after all. I would argue this is a perfect place for a mindless code should take after.

Out of curiosity, is there any specific reason why a "page-server-only" route should not be included? After all, a +page.server.js file is sufficient for a sveltekit app to respond to a HTTP request.

jycouet commented 4 months ago

I'm not sure what to answer here as I have many questions 😅

A/ Are you sure svelteKit redirect can redirect to an external link? I just tried, and redirect(302, 'kit.svelte.dev') goes to http://localhost:5173/kit.svelte.dev Maybe with another code ? (I don't know)

B/ Why do you want to redirect to /login/github that will then redirect to /my-other-link ? Why not going directly to /my-other-link ?

C/ if you run npm run check with an invalid param like: route('/redirect-me'), the cli will tell you. (So you catch thing in dev, not in prod)

D/ Let's think that now each load is supported in the route function (like you want). What happen to people only returning data ? They will have a route("/something") that is valid, but where people can't go to! And at runtime, svelte will write: Error: Missing +page.svelte component for route /something

E/ Out of

Out of curiosity, is there any specific reason why a "page-server-only" route should not be included? After all, a +page.server.js file is sufficient for a sveltekit app to respond to a HTTP request.

Is it? to respond to a HTTP request, yes, but not to a user route. It will return you json, not a html page. 🤔

F/ Out of

Declaring it in a separate config file sounds appropriate if I have plans to use it in multiple places, or if it may change in the future more frequently than I planned. Otherwise, it feels a bit much.

Right now you are creating a separate file +page.server.ts to have one redirect... So it's about the same ?

I feel like I can live without using the route() function at all.

A lot of people do live without this route() function, it's ok 😉


Thrilled to read the next step ✌️ // Happy coding

jangxyz commented 4 months ago

I gave some more thoughts on it, and before answering each specific questions, I realized that we have different views on what +page.server.js should do.

I realized that I see it as some kind of controller, like in traditional MVC sense. Sure, it does prepare data for the page component to render, but it can do more, like taking care of cookies, and some redirects as well. That is why I thought it is sufficient to regard as a controller in charge when requested with a specific url, hence should be considered as a route, even without a view(component). It's not common, but possible for a route to handle things besides rendering. As a far fetched example, think of a svelte component without rendering any HTML tags, but only some logics running onMount() with a <slot />. It's not a everyday thing, but completely valid.

Whereas, I felt that you are seeing it as literally a load function that loads data for the component to use. In this sense, the main purpose of existence is hard to think without the existence of the page component. After all, that's how it is documented in the first place:

a +page.svelte component (...) can be rendered, (...) get some data. This is done by defining load functions. -- Loading data, SvelteKit Docs

In this point of view, other jobs should be done through actions.

As a firm believer of the religion, I do not have any doubts in the grand scheme of SvelteKit 😁 . I just did not realize the view I originally had was different. In my view, the distinction between load and action are hardly visible. In this regard, if having a 'form action' function qualifies for registration in AllTypes, then a load function should also qualify, regardless of whether it has a component or not.

Now let me answer your questions.


A/ Are you sure svelteKit redirect can redirect to an external link?

I just tried, and redirect(302, 'kit.svelte.dev') goes to http://localhost:5173/kit.svelte.dev Maybe with another code ? (I don't know)

Adding the protocol makes it work.

My actual code in use is:

async function load({ cookies }) {
  const url = await github.createAuthorizationURL(state)
  return redirect(303, url.toString())
}

and it redirects to `https://github.com/login/oauth/authorize?response_type=code&client_id=...&state=...'

B/ Why do you want to redirect to /login/github that will then redirect to /my-other-link ? Why not going directly to /my-other-link ?

There are some scenarios where you need to have additional links before redirecting to somewhere else, be it internal or external. In my code, I set a cookie from the server beforehand, so that I can verify the code from the callback later. I might need it for logging for analytics as well. In other cases, I've seen /logout links that removes the session cache, and redirects to the root page. (Maybe better to handle it within a POST action? Maybe, but whether to allow an action to be GET or POST is left for the service to decide). An unpleasant experience, but I've also seen a large IT company having too many divisions in itself that they were redirecting against each other, like 3 or 4 redirects across different domains, just to login :(

And there's good'ol "just to keep the urls clean". Someone might be managing legacy urls, or like in my case preparing a structure for future expansion (for instance planning to support google login).

The point is, that there can be any application-specific or business related logic that decides to have another link.

C/ if you run npm run check with an invalid param like: route('/redirect-me'), the cli will tell you. (So you catch thing in dev, not in prod)

Yes, I agree. (Have I said it otherwise?)

D,E/ page with data but without a component

Let's think that now each load is supported in the route function (like you want). What happen to people only returning data ? They will have a route("/something") that is valid, but where people can't go to! And at runtime, svelte will write: Error: Missing +page.svelte component for route /something

Out of curiosity, is there any specific reason why a "page-server-only" route should not be included? After all, a +page.server.js file is sufficient for a sveltekit app to respond to a HTTP request.

Is it? to respond to a HTTP request, yes, but not to a user route. It will return you json, not a html page. 🤔

Yes, you are right that a loading data without a page component would result in error.

I was referring to a redirect-only page, which in this case would work without a page component. In this case not having a +page.svelte file is okay for sveltekit.

NOTE The point that a 'request handler' having solely redirect mechanisms and no data may sound like it should be better put in +server.js. That is what I actually tried in the first place. To my disappointment, I found out that it does not react to link clicks from a page, and that's how I ended up in +page.server.js.

F/ Having a separate config

Declaring it in a separate config file sounds appropriate if I have plans to use it in multiple places, or if it may change in the future more frequently than I planned. Otherwise, it feels a bit much.

Right now you are creating a separate file +page.server.ts to have one redirect... So it's about the same ?

There seems to be a few steps I can take here..

I know workarounds exist. I guess in my "controller" point of view, I thought that it is more natural for the route plugin to follow the development flow. Additional processes like step A would be troublesome, even more sveltekit has no problems without it. Step B feels less, but I thought I could expect more.


Once again, thanks for the plugin. I really like the plugin, and the magicfulness it brings to the framework :)

jycouet commented 3 months ago

Thanks for all your details, I love it ✌️


NOTE The point that a 'request handler' having solely redirect mechanisms and no data may sound like it should be better put in +server.js. That is what I actually tried in the first place. To my disappointment, I found out that it does not react to link clicks from a page, and that's how I ended up in +page.server.js.

Reading your text, made me think about +server.ts too! To bad that it's not working as you expect since it's taken into account in kit-routes plugin.

a) Add a blank +page.svelte file: maybe this will 'notify' kit-routes to consider this as a valid route. (Not sure, will it?)

Yes, this should work. (Not that nice to have this empty file to ensure the functionality...) Maybe add in the page something like: "Needed for Kit Route plugin to pick up the route and allow route("/tadaa") to work!" (so that you remember in 6 months)

b) Add LINKS property in config each time.

I would go for this option in your case I think.


If you agree, I would close the issue.

jangxyz commented 3 months ago

Yes, I understand now we got different views, and I can follow along with kitql's work :)

Thanks for the kind conversation!