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

Rename LoadInput to LoadEvent #4965

Closed
felixsanz opened this issue May 17, 2022 · 6 comments · Fixed by #5015
Closed

Rename LoadInput to LoadEvent #4965

felixsanz opened this issue May 17, 2022 · 6 comments · Fixed by #5015
Milestone

Comments

@felixsanz
Copy link

felixsanz commented May 17, 2022

Describe the bug

The endpoint's get function doesn't have type for input arguments. In javascript you can use the generated types, but that's not possible with typescript. So the whole object or individual keys should be able to type. Example:

export async function get({ params }: ThereIsNoTypeAvailable): Promise<RequestHandlerOutput> {}
export async function get({ params }: { params: ThereIsNoTypeAvailable }): Promise<RequestHandlerOutput> {}

The output is fine, RequestHandlerOutput is available and we can use it.

Yeah, you can type a function expression like this:

export const get: RequestHandler = async ({ params }) => {}

But it's impossible to type function statements right now. There's RequestEvent but it's not exported (it's private).

On the other hand, the load function is fine:

  export async function load({ params, fetch }: LoadInput): Promise<LoadOutput> {}

You can use the Load type on a function expression or LoadInput + LoadOutput on a function statement.

So we should have consistency and have both input and output types available in functions, specially get.

load() get()
Load RequestHandler
LoadInput ????????
LoadOutput RequestHandlerOutput

Maybe RequestHandlerInput ?

Reproduction

NA

Logs

No response

System Info

NA

Severity

serious, but I can work around it

Additional Information

NA

@Rich-Harris
Copy link
Member

I think we should make RequestEvent public. For consistency I wonder if we should rename LoadInput to LoadEvent.

You can use generated types with TS, though right now you need to add .d, like this:

import type { RequestHandler } from './index.d';

That'll be fixed by #4705, once we agree on exactly how it should look.

@Rich-Harris Rich-Harris added this to the 1.0 milestone May 17, 2022
@felixsanz
Copy link
Author

I think we should make RequestEvent public. For consistency I wonder if we should rename LoadInput to LoadEvent.

You can use generated types with TS, though right now you need to add .d, like this:

import type { RequestHandler } from './index.d';

That'll be fixed by #4705, once we agree on exactly how it should look.

I think for consistency it should be LoadInput/LoadOutput and RequestHandlerInput/RequestHandlerOutput.

Because the arguments are the function's input. And an event imo is the whole function when it's called.

@Rich-Harris
Copy link
Member

And an event imo is the whole function when it's called.

You can think of a request handler (either in an endpoint, or the main handle hook) as being sort of analogous to a DOM event listener:

addEventListener('GET', event => {
  // ...
});

The argument to the handler callback is always called event (or evt or e), not input, and the 'event' terminology refers unambiguously to that argument, not to the addEventListener call or the handler or the invocation of the handler.

The same convention is used inside AWS Lambda:

exports.handler =  async function(event, context) {
  return {...};
};

So RequestEvent and event are the right names; I think the only question is whether to rename LoadInput to LoadEvent (I would say yes).

@Rich-Harris Rich-Harris changed the title Kit lacks some exported types for function statements arguments Rename LoadInput to LoadEvent May 18, 2022
@Rich-Harris
Copy link
Member

I just merged #4809 which makes RequestEvent public, so I've edited this issue to 'Rename LoadInput to LoadEvent'

@felixsanz
Copy link
Author

Then... if this functions (get(), post()... load()) are attached/subscribed like a listener, then Event is appropriate. If they are just called... i don't think so.

I'm kinda new and don't know the sveltekit internals, but probably it just boils down to that.

@tcc-sejohnson
Copy link
Contributor

@Rich-Harris

Making private types public seems to be my purpose in life. Maybe I should branch out to renaming public types...

If you'd like to rename LoadInput to LoadEvent, I'll make a PR. Just let me know your final decision. 😉

Rich-Harris added a commit that referenced this issue May 23, 2022
* rename LoadInput to LoadEvent - closes #4965

* Update packages/kit/types/index.d.ts

Co-authored-by: Maurício Kishi <mrkishi@users.noreply.github.com>

* Update packages/kit/types/index.d.ts

Co-authored-by: Maurício Kishi <mrkishi@users.noreply.github.com>

Co-authored-by: Maurício Kishi <mrkishi@users.noreply.github.com>
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.

3 participants