Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

request.params not available in handle hook as docs / types indicate #1194

Closed
RaeesBhatti opened this issue Apr 23, 2021 · 10 comments · Fixed by #4344
Closed

request.params not available in handle hook as docs / types indicate #1194

RaeesBhatti opened this issue Apr 23, 2021 · 10 comments · Fixed by #4344
Milestone

Comments

@RaeesBhatti
Copy link

Describe the bug
Request.params is null in handle hook.

Logs

To Reproduce

  1. Create a route that uses params, e.g. src/routes/[product]/index.svelte
  2. Create a hook with handle function located at src/hooks.ts
  3. Try to access request.params in handle function

Example repo: https:/RaeesBhatti/svelte-params-bug

Expected behavior
request.params should be available in handle function of hooks.

Stacktraces

Information about your SvelteKit Installation:

Diagnostics
  System:
    OS: macOS 11.2.3
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 535.77 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.8.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.7 - /usr/local/bin/npm
  Browsers:
    Chrome: 90.0.4430.85
    Safari: 14.0.3
    Safari Technology Preview: 14.2
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.87 
    svelte: ^3.29.0 => 3.37.0 
  • Your browser: Safari Version 14.0.3 (16610.4.3.1.7)

  • Your adapter: Cloudflare Worker but this issues comes up even in dev mode.

Severity
Blocking usage of SvelteKit because I need params to fetch the right data from database.

Additional context

@RaeesBhatti
Copy link
Author

I think I understand the reason behind this. I tried to make it work but handle only executes once per request instead of executing on each possible route being tried. So, it's not possible to provide the correct params during handle execution.

My use case is that I want to load some data from database and have it available in the context but Incoming doesn't have params property.

@ghost
Copy link

ghost commented Jun 7, 2021

Can we reopen this please? The docs mistakenly mention that params is a actually available and I have another use case for this which is obscuring IDs from your database with something like Hashids. Without params inside handle you would have to decode your params in every endpoint route which is quite inconvenient. Maybe there is room for another hook that is executed once the a matching route has been found.

@benmccann benmccann reopened this Jun 7, 2021
@benmccann benmccann added this to the 1.0 milestone Jun 7, 2021
@benmccann benmccann modified the milestone: 1.0 Aug 5, 2021
@benmccann
Copy link
Member

Hmm. I was wondering if we should move it until after handle is called, but I think that doesn't really work with route fallthrough because we have to actually execute load on each route to know what final route is rendered

For reference, right now it's called here:

return await options.hooks.handle({

@benmccann benmccann removed this from the 1.0 milestone Aug 5, 2021
@kamholz
Copy link

kamholz commented Sep 2, 2021

What's the current recommended way to deal with this? I have a bunch of endpoints with an [id] param and I need to short-circuit them and return an error if the param is not in a valid format. I was hoping to do it in handle but it looks like I need to add this boilerplate to every handler (get, post, del, etc.) of every endpoint. Is there no better way? The only way I can think of is to write a function that wraps every handler, so:

export const get = validateParams(async ({ params }) => { ... });

I suppose this is workable but seems pretty clunky.

@benmccann benmccann added this to the 1.0 milestone Sep 3, 2021
@benmccann benmccann changed the title request.params not available in handle hook request.params not available in handle hook as docs / types indicate Sep 30, 2021
@nhe23
Copy link
Contributor

nhe23 commented Jan 11, 2022

As using params in the handle hook does not make much sense this should just be removed from the docs. If endpoints are accessed via load function then validation or other logic including params could be run prior to calling the endpoints (e.g. in a __layout.svelte). However I think there are valid usecases (as pointed out by @kamholz and @jordankaerim) to have a mechanism to execute code before endpoints are executed.
There could be a global handleEndpoints hook that runs before executing the endpoint:

const response = await handler(request);

This hook could implement some middleware logic which could make something like this possible:

I have another use case for this which is obscuring IDs from your database with something like Hashids. Without params inside handle you would have to decode your params in every endpoint route which is quite inconvenient. Maybe there is room for another hook that is executed once the a matching route has been found.

Endpoints themselves could also export some kind of function (not sure what to call it ... maybe init or also handleEndpoint) that runs before the method function is executed and also acts like a middleware. This way it would be possible to have endpoint specific logic (e.g. validation) that is always executed when calling endpoints.

This solution would improve the above discussed usecases ... however I'm not quite sure if it's really a good idea to do it this way. So please let me know what you think...

@sjones512
Copy link

I would like to know what the intended method is.

If I have routes /foo/[bar]/bat and /foo/[bar]/baz and each one supports multiple methods GET, POST, DELETE and I want to guard the routes based on the current user and the value of bar I will currently need to implement that in every RequestHandler?

My initial thought was I might be able to make a wildcard endpoint at /foo/* that would match all sub routes. I thought maybe I would be able to have it do validate/authorization logic and then fallthrough. But I think I was incorrect in understanding how the paths are resolved, and don't think a path like that would ever get matched over the more specific matches. Even if that were the case I believe I would have to implement separate handlers for each HTTP method.

My next approach was to try doing the validation and authorization in a Handle hook, but then I realized that the params aren't available in the hooks. At least it doesn't appear they are there until after resolve is called, so I don't think that will work.

My current implementation is @kamholz's solution of creating a wrapper function. As stated this kind of works, but still creates the mental overhead of making sure to wrap every handler. Is this the best solution? I'm still new to sveltekit, is there another solution I'm overlooking? Am I just approaching this wrong and should rethink my routes/endpoints?

@georgecrawford
Copy link
Contributor

To chime in to this thread. I agree that params should be removed from the docs for the handle() hook, as it doesn't make sense here.

BUT, I'd love a way to hook in to the route and params which produced a response, where it was successful. This would be phenomenally useful in implementing monitoring (I use Prometheus, for example), where typically a label is added to all response logs like {method: 'GET', route: '/users/[id]/details', status: 200}, and means I can aggregate all similar requests to the same route and understand how long the response took to generate. At present, since I don't know which route rendered the response in handle(), the best I can do is to use url.pathname, which means I can't aggregate across routes.

Perhaps we could consider a handleResponse hook, which receives an immutable Response which contains (somehow???) details that SvelteKit has added to indicate the successful route and params.

Happy to open a separate issue for this request if necessary, but it's worth mentioning here as a use-case I think.

@sjones512
Copy link

@georgecrawford I believe the params are populated when you call resolve so you should have access to them, and the response status after calling resolve.

@georgecrawford
Copy link
Contributor

You're right - params is available after the resolve is complete, but only if you destructure it from event after that resolve:

    const {params} = event;
    console.log('before', event.params, params);
    const response = await resolve(event);
    console.log('after', event.params, params);
curl https://localhost:3000/route/value

gives:

before {} {}
after { paramName: 'value' } {}

I'd suggest that event.params should be assigned to, not overwritten, to make this more useful.

And I still don't know which route served this response, which is the main thing I'm looking for in #3840

@georgecrawford
Copy link
Contributor

See related discussion on param validators here: #4291

Rich-Harris added a commit that referenced this issue Mar 16, 2022
* remove fallthrough

* changeset

* remove fallthrough documentation

* tweak docs

* simplify

* simplify

* simplify a tiny bit

* add failing test of param validators

* client-side route parsing

* tidy up

* add validators to manifest data

* client-side validation

* simplify

* server-side param validation

* lint

* oops

* clarify

* docs

* minor fixes

* fixes

* ease debugging

* vanquish SPA reloading bug

* simplify

* lint

* windows fix

* changeset

* match route before calling handle - closes #1194

* changeset

* throw error if validator module is missing a validate export

* update configuration.md

* Update documentation/docs/01-routing.md

* tighten up validator naming requirements

* disallow $ in both param names and types

* changeset

* point fallthrough users at validation docs

* add some JSDoc commentsd

* expose `event.routeId` and `page.routeId` (#4345)

* expose event.routeKey - closes #3840

* change routeKey to routeId

* rename routeKey to routeId, expose page.routeId

* rename route.key -> route.id everywhere

* adapter-node sure picked a weird time to stop typechecking

* oops

* Update .changeset/mean-crews-unite.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants