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

lint against throw Fail #10118

Closed
turadg opened this issue Sep 19, 2024 · 3 comments
Closed

lint against throw Fail #10118

turadg opened this issue Sep 19, 2024 · 3 comments
Labels
devex developer experience technical-debt

Comments

@turadg
Copy link
Member

turadg commented Sep 19, 2024

What is the Problem Being Solved?

It's redundant and seeing it trains people to be lax in when errors are created vs thrown.

Description of the Design

Fail is only for the condition || Fail pattern.

throw Fail is not allowed.

Bonus points for auto-fixing to throw makeError

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

@michaelfig
Copy link
Member

michaelfig commented Sep 19, 2024

The reasons to use if (!condition) throw Fail`xxx`; instead of condition || Fail`xxx`; or if (!condition) throw makeError(...); are:

  1. There's a Typescript deficiency where condition || functionReturningNever() does not properly narrow types implied by condition in the subsequent statements, but if (!condition) functionReturningNever(); does.
  2. There's a Typescript deficiency where calling a template tag that returns never (like Fail) is not treated the same as throw ... (i.e. it isn't handled as a terminal statement in the control-flow graph).
  3. makeError only redacts (like Fail does) if you use makeError(redacted`xxx`) (also aliased as makeError(details`xxx`) or makeError(X`xxx`)).

If you really want to weed out the redundancy of throw Fail`xxx`;, I'd propose a new errorWith (name to be bikeshodden) template tag that would compose both makeError and redacted so throw errorWith`xxx`; is equivalent to throw makeError(redacted`xxx`);. With some trickery, you could smuggle other arguments into makeError so that throw errorWith(...otherArgs)`xxx`; is equivalent to throw makeError(redacted`xxx`, ...otherArgs);.

@turadg
Copy link
Member Author

turadg commented Sep 19, 2024

Closing this for now with the expectation that 51426 will be fixed eventually. Then we can ban throw Fail but we won't have to come up with a replacement.

@turadg turadg closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience technical-debt
Projects
None yet
Development

No branches or pull requests

2 participants