-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: uppercase endpoint methods #5513
feat: uppercase endpoint methods #5513
Conversation
🦋 Changeset detectedLatest commit: 3ba4c2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The tests were more regular than I thought they would be, nice. I found this while I was taking a quick peek the other day: kit/packages/kit/src/vite/build/build_server.js Lines 258 to 295 in 606b9d6
It's just passed to adapters so couldn't exactly tell what it's doing using |
Regex find and replace, buddy. 😉 Good catch -- I updated those two files along with the type definition (so we can be reasonably certain there are no lowercase variants remaining). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great to me, i just made a couple of small suggestions
…-sejohnson/kit into sejohnson-uppercase-endpoint-methods
@@ -409,13 +409,13 @@ async function load_shadow_data(route, event, options, prerender) { | |||
try { | |||
const mod = await route.shadow(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: Remove for 1.0 | |
['get', 'post', 'put', 'patch', 'del'].forEach((m) => { | |
if (mod[m] !== undefined) { | |
const replacement = m === 'del' ? 'DELETE' : m.toUpperCase(); | |
throw Error(`Endpoint method "${m}" has changed to "${replacement}"`); | |
} | |
}); | |
The error should be here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yep, i'll rustle up a utility that can be used in both places
For my own curiosity, @Rich-Harris what's the output of the get_methods function used for? Couldn't find anything in the repo. Also might be nice to add a doc comment. |
This is... a good question. The only place it appears to be referenced is here: kit/packages/kit/src/core/adapt/builder.js Lines 40 to 51 in 5eb7652
I vaguely remember there being some reason that it was useful for adapters to have this information to hand. Perhaps if you're using Architect, where you need to define routes like this:
|
alright, let's go... hopefully the positive emoji reactions on the discussion and the issue are representative of the community as a whole 🤞 |
- Capitalize endpoint method names from sveltejs/kit#5513 - Upgrade e2e Banananation from NASA-AMMOS/aerie#238
- Capitalize endpoint method names from sveltejs/kit#5513 - Upgrade e2e Banananation from NASA-AMMOS/aerie#238
- Capitalize endpoint method names from sveltejs/kit#5513 - Upgrade e2e Banananation from NASA-AMMOS/aerie#238
Proof-of-concept implementation of #5367.
The server will throw an error if an endpoint module exports one of the old names. This behavior should be removed for 1.0 (and is marked with a comment like other pre-1.0 throws are).
Closes #5367.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0