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 invalid to... something else? #7982

Closed
Rich-Harris opened this issue Dec 7, 2022 · 24 comments · Fixed by #8012
Closed

Rename invalid to... something else? #7982

Rich-Harris opened this issue Dec 7, 2022 · 24 comments · Fixed by #8012

Comments

@Rich-Harris
Copy link
Member

Describe the problem

One side-note from #7956: invalid(422, {...}) is natural, but invalid(500, {...}) — which you need to do in order to show inline error UI along with repopulated form values — feels a bit weird. The error isn't that the data was invalid, it's that something went wrong on the back end.

Describe the proposed solution

Rename it to something that covers both 4xx and 5xx errors:

  • fail(status, data)
  • failed(status, data)
  • nope(status, data)

Suggestions welcome. (Note that we can't return error(status, data) because data must conform to App.Error in that case.

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

@amrnt
Copy link

amrnt commented Dec 7, 2022

halt(status, data)?

@sourcegr
Copy link

sourcegr commented Dec 7, 2022

Boom(status, data)

@dummdidumm
Copy link
Member

I like invalid, because it's a very good fit for 90% of use cases.

@smstromb
Copy link

smstromb commented Dec 7, 2022

+1 for failed(status, data)

@arackaf
Copy link

arackaf commented Dec 7, 2022

You could keep invalid as it is, but add a failed() which took only the error data, and returned a 500 for you. Ie

invalid(420, { status: "Too high" });

or

failed({ status: "Something broke }); // returns a 500 

@sourcegr
Copy link

sourcegr commented Dec 7, 2022

on a second thought, I kind of regret the boom proposal due to some reasons.

  • it feels a little cryptic
  • it is kind of informal
  • is used by hapi and it might bring confusion over time

So, I would rather recommend a more generic failure(status, data) instead of fail, failed

return failure(status, data) seems more natural

@scottBowles
Copy link

fauxpas, please.

Really though, I like either invalid or failure. I'm mostly ok with invalid because it describes how you're choosing to handle the error. The invalid function takes some backend reality and decides it will hand it to the client-side +page to handle it as a bad response, rather than as the +error page. If it wraps a 500 that seems fine to me because the 500 describes what went wrong, and the use of invalid describes how we're passing it forward. I could see failure too, and in any case ValidationError should probably be changed.

Alternatively, I could see changing error/invalid to something like panic/error to capture that one is disrupting the usual flow of your app and the other is passing on a bad message.

@sourcegr
Copy link

sourcegr commented Dec 7, 2022

And while we 're at it, may I boldly suggest we revisit the naming for validationError, since this could also be an upstreamError or a connectivityError or someOtherReasonError

@sourcegr
Copy link

sourcegr commented Dec 7, 2022

handlingError would also be a fits-both proposal, and also solves the interface naming problem for ValidationError; it is renamed HandlingError.

OK, I am going to shut up now :)

@cdcarson
Copy link
Contributor

cdcarson commented Dec 7, 2022

Keep invalid. This means what it says -- you have an invalid form that the user can fix in the context of the form itself. Overloading that with errors like

  • "make sure your WiFi is in range" or
  • "hey, bad actor, you can't launch the missile, you should never have even seen that button"

is IMO bad UX and bad DX.

The fact that 400 Bad Request and 403 Forbidden share a 4 is academic -- what matters is the difference in the business logic.

@kvetoslavnovak
Copy link
Contributor

fail(status, data) sounds ok

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

I like fail (failed feels wrong -- it would imply we should also have errored and redirected, which sound terrible). I like nope most of all, but I also know the serious-minded folk would hate it. 😆

@sourcegr
Copy link

sourcegr commented Dec 7, 2022

Actually, there is one more aspect to this...

The problem is that invalid is the wrong term to describe a 5xx error. But on the other hand, invalid is a perfect fit for the 4xx, and I would not be very pleased if I had to live without it, let alone it would be confusing if we replace it with something errorish...

So, maybe it would be better to keep the invalid and add an alias to it with the new name.

This way, the following code would be much more self explanatory and a pleasure to read, leaving no room for misunderstanding.

import { invalid, failure } from '@sveltejs/kit';

export const actions = {
  setName: async ({ request }) => {
    const data = await request.formData();
    const name = data.get('name');

    if (!name) {
      return invalid(400, { name, missing: true });
    }
    try {
      result = await postToExternalAPI(name);
    } catch(error) {
      return failure(504, {name, message: 'Upstream server coul not be reached. Try again later'});
    }

    return { success: true };
  }
}

@mavdotjs
Copy link

mavdotjs commented Dec 8, 2022

+1 for nope makes coding a bit more fun

@selimhex
Copy link

selimhex commented Dec 8, 2022

Considering 4xx and 5xx status codes being for all client and server error responses,invalid is indeed insufficient to cover even the 4xx errors. Since we're literally returning either a client or server error and the word is apparently taken, how about a synonym?

Among many funny or fancy options, i think a single synonym stands out: failure. It covers the exact meaning, is neutral (whose fault?), short enough and is a common word, which is good for non-native english speakers.

@yousufiqbal
Copy link
Contributor

I prefer fail(status, data). This is better naming convention wise.

@shalior
Copy link

shalior commented Dec 8, 2022

how about reject(status, data)

@PatrickG
Copy link
Member

PatrickG commented Dec 8, 2022

Rename error to exception and invalid to error.
The migration would be funny 🤣

On a serious note, of the suggestions, I like failure the best.

@zachsa999
Copy link

zachsa999 commented Dec 8, 2022

failure please

@Rich-Harris
Copy link
Member Author

Opened a PR that renames invalid(...) to fail(...) and ValidationError to ActionFailure#8012.

Having gone through the process of updating the code, I'm convinced that this is the right move even if it's a late breaking change. Another couple of reasons presented themselves:

  • 'invalid' is used elsewhere in the framework to refer to data that is out of date and needs to be refetched. It's arguably confusing to have invalid(...) and invalidate(...) to refer to different meanings of the word
  • The type of an ActionResult can be success, failure, redirect or error. failure is a better counterpart to success than invalid. (Also, all four are now nouns, instead of three nouns and an adjective. It's a happy coincidence that redirect and error are also verbs*, making them suitable as function names as well.)

*if someone wanted to be persnickity they could challenge the notion that 'error' is a verb, but I think it's used as a verb often enough in programming that I can claim it with a straight face

@dummdidumm
Copy link
Member

Still not fully convinced. In the ActionResult example I now have a harder time to distinguish between error and failure, since they are much closer connected.

@sourcegr
Copy link

sourcegr commented Dec 9, 2022

it just feels that

return failure(...

is more natural than

return fail(...

@524c
Copy link
Contributor

524c commented Dec 9, 2022

I like this

return failure(status, data)

or something shorter

return failed(status, data)

And I agree that invalid is Ok for 4xx, but it doesn't match anything with 5xx.

Rich-Harris added a commit that referenced this issue Dec 9, 2022
* rename invalid() and ValidationError - closes #7982

* help people migrate
@simonsarris
Copy link
Contributor

glad you picked a verb :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.