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

Uppercase handler names (export function GET() {...} etc) #5367

Closed
Rich-Harris opened this issue Jul 5, 2022 · 7 comments · Fixed by #5513
Closed

Uppercase handler names (export function GET() {...} etc) #5367

Rich-Harris opened this issue Jul 5, 2022 · 7 comments · Fixed by #5513
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

Details in this discussion. Consensus seems to be that this is a positive change.

Describe the proposed solution

Replace this...

// src/routes/some-endpoint.js
export function get({ params }) {...}
export function del({ params, request }) {...}

...with this:

// src/routes/some-endpoint.js
export function GET({ params }) {...}
export function DELETE({ params, request }) {...}

Alternatives considered

Not doing this.

Importance

nice to have

Additional Information

No response

@Rich-Harris Rich-Harris added this to the 1.0 milestone Jul 5, 2022
@UltraCakeBakery
Copy link

UltraCakeBakery commented Jul 7, 2022

I removed my previous comment because I thought I misread something, but I now think I didn't. How about making the names of handler functions not case sensitive, and allowing users to export any function from an endpoint and with that support any method? We could make exceptions for functions that start with a _ or have a reserved keyword.

@Rich-Harris
Copy link
Member Author

No, that's a bad idea. APIs should be as explicit and non-magical as possible — loosey-goosey things like accepting get or GET or Get or gEt create confusing documentation and pointless debates over best practice

@UltraCakeBakery
Copy link

No, that's a bad idea. APIs should be as explicit and non-magical as possible — loosey-goosey things like accepting get or GET or Get or gEt create confusing documentation and pointless debates over best practice

If you are talking about the APIs that users are building with SvelteKit, that should be left up to them and not SvelteKit. If you are talking about SvelteKit's api, I guess you're right. I'm not a fan of arbitrary limitations though and many others care about not having multiple naming conventions in their codebase.

Maybe it is time to consider taking the same approach other file based routers have taken where you create endpoint.method.js files that export default () => {}. That way you leave the writing of the method to the file name. SvelteKit maybe should throw a warning when you are using a unsafe http method instead. That way you don't have to document anything else besides "http methods that are not considered SAFE will throw a warning, but still work for backwards compatibility and flexibility". The warning can be something along the lines of X is not a safe http method. { method.toUpperCase() === method ? '' : 'Methods are case sensitive'}. Did you make a typo?. Maybe not the most thought out idea but I like this direction more because it will also push people to put shared logic between endpoints in their $lib folder.

Anyways, I do not have a big stake in this change at all. I can live with the changes. I'll survive for another day lol. Just throwing out some alternatives here.

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jul 9, 2022

having endpoint.method.js would mess with file structure so much,... it's already mess with layouts... please don't....

I agree on named exports, the only thing is if it's better having awkward del or http-ish DELETE that as name in JS evoke it's constant.

But I use del not that often as get and post, so... I don't know if del is too "bad" for letting it be as it is now. Or if we really want DELETE instead.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

I was just playing around with this and it turned out to be surprisingly easy to implement. I did the implementation and also updated all of the tests and documentation with the new method names. There's a PR here: #5513.

I meant to mark that PR as a draft (since it's not clear whether this is settled on or not), but it looks like I can't change it now.

@Conduitry
Copy link
Member

GitHub permissions are highly mysterious. I've just marked that PR as draft, but for the record, I am in favor of this change.

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jul 14, 2022

Snímka obrazovky z 2022-07-14 10-58-28

not only thing that is mysterious.... I want to give you 👍 , but I can't.... clicking on existing one does nothing.

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.

5 participants