-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Tuples/Objects and Falsy errors. #30
Comments
This comment was marked as outdated.
This comment was marked as outdated.
I'd make a few arguments for an instance of some object type.
Even w/o TS the ability to extend Iterables (not tuples per JS spec nomenclature!) is more limited and requires iteration of all keys to get to the value desired, compare: let foo = () => { /*...*/ ; return undefined; }
let obj ?= foo()
if (obj.threw) {
// ...obj.error
}
let [err, value] ?= foo()
if (err) {
// potentially was thrown for falsy values
} Helpers for specific case detection would not really have API space in the iterator form. |
aesthetically I prefer tuple |
That's interesting, because I find them aesthetically unpleasant. There have been arguments throughout for why they are functionally better for this purpose, so I'm not saying they are wrong, but aesthetically, they look dumb to me. Because they assume what the operation/expression will return. It's like a fun "let's just see if this works" style of setting variables. Maybe this is part of what I find disjointed about the entire proposal, is that deconstructed assignment doesn't look stable or sensible to me (from a distance). I'm sure they'll carve all of my personal opinions about coding on my tombstone. But as long as you bring up what we prefer aesthetically, I figured I'd chime in. |
i kinda see what you tryna say, but the same apply for a object, maybe your problem are with destructuring |
I think it would be safer to use an exotic object or constructor like Error or Response due the positional nature of "tuples". I don't know... SafeOperation?
const n: SafeOperation ?= fn(); Where: type SafeOperation = { error, result }; Allowing: const { error, result } ?= fn(); // SafeOperation type/constructor infered by the operator I think an exotic object also does make sense since JS doesn't really have tuples (just another exotic object: Array) that can lead to strange behaviors (#5 (comment)). SafeOperation In the future can extend its own properties depending on the context, e.g.: Error which may have a Also as I mentioned, would be cool to use it like a Error or Response constructor: const n = new SafeOperation(error, result); // constructs a new SafeOperation object which error and result properties Or const n = new SafeOperation(fn()); // constructs a new SafeOperation resultant of the fn(); |
I'm drawn to the idea of wrapping non-Error thown values in a dedicated Error and returning a tuple. I think it would be a very nice way to deal with sync errors. I just can't see the TC39 committee going for it. I'm not exactly sure why but something about coercing thrown values doesn't feel JavaScript. |
Even though I aesthetically prefer the object, I would go with tuples for a few reasons:
However, to @felippe-regazio's comment, I think it's also a possibility to think, either adding it as |
Exactly, I also think that tuples could be a nicer option than using object destructuring because this lets people re-assign value names, thus allowing the following pattern to emerge: const [authorsError, authors] ?= await fetch("/authors")
if (authorsError) { /* handle error */ }
const [booksError, books] ?= await fetch("/books")
if (booksError) { /* handle error */ } |
const { error: authorsError, result: authors } ?= await fetch('/authors');
const { error: booksError, result: books } ?= await fetch('/books'); |
One thing I keep thinking about is this existing pattern for async error handling. Something like this for synchronous errors would be really nice. const books = await fetch("/authors").catch((error) => {
/* handle error */
}) |
The problem with the above pattern is that it mixes the success and rejection into the same variable, making it hard to differentiate afterwards in case a default value could not be provided. |
Yeah that's my point number 2, we have to repeat the original names on both objects to change the names whereas with tuples we could use: const [authorsError, authors] ?= await fetch('/authors');
const [booksError, books] ?= await fetch('/books'); Objects are indeed more explicit, which is nice, but I think that, in this particular case, it's already explicit in the construct (especially if we use |
const [reqErr, response] = try await fetch(url); This is beautiful syntax 😄❤️ It's much more readable and self-explanatory than the cryptic two-for-one |
const [response, error] = try await fetch(url); This syntax is optimal as it aligns well with the throw expression proposal, which is currently in Stage 2. The proposed syntax is self-explanatory, making error handling more concise and readable. |
I work with async functions a lot and if I can't return a default value in a catch statement then I either throw an error or return undefined. If I return undefined, I can just check for that with an if statement and gracefully exit the parent function. Usually the catch is where I handle the error and if I can't handle it there I probably can't handle it anywhere. So it's essentially the same thing. It's doubtful that I would use try expressions for most async functions since the async...await syntax already handles it perfectly, but it would be a useful shortcut for some of the typing shenanigans that can result. |
Throwing now we just need find a way to handle it in this operator. we could [ok, error, data] or make error be a TypeError when the thrown error is undefined or null |
The latter is not an option; if a value is thrown, that's the error value, full stop. |
I think if we did that it should be The only other option would be to return a TryError with the error as a property whenever an error gets thrown. The TryError stacktrace would be to the point of the try keyword, and the error property would contain whatever value was thrown by the code (usually an Error instance). |
Just my $.02, but why design for an anti-pattern? Throwing a value is unusual and very bad practice. I would not suggest reusing I would suggest adding a new This will make the return type simpler, e.g. This would also enable improvements to test frameworks and error handlers, which are currently unable to obtain a stack-trace from bad code that throws a value. Fix a problem, rather than propagating it. |
Actually it makes sense to throw an error if Additionally we can get an issue with test-runners: jestjs/jest#2549 |
You want the This would make no sense to me.
What I'm suggesting is, the
What issue? (what you linked to is something about type-checking arrays?) Test runners wouldn't need to use the |
oh! My bad, I ment to return the one in this case.
We can wait for the Error.isError proposal to resolve this. To wrap falsy values could be safer because it will allow to check
Oh, the |
yes, the old discussion. Ok, let's look on the following case: const [ok, error, fileName] = try await getFilename();
fs.writeFileSync(`__dirname/${fileName}`, 'Hello World'); Are something screaming here? Actually TS also accepts this broken code |
let filename
try {
filename = await getFilename()
} catch(error) {
console.log(error)
}
fs.writeFileSync(`__dirname/${fileName}`, 'Hello World'); Will also pass on TS checks but a lot of linters have rules to prevent this problem. Even though only let filename /* undefined */
if (conditionThatNeverMets) {
filename = 'something'
}
fs.writeFileSync(`__dirname/${fileName}`, 'Hello World'); I see that the above is a problem because const [ok, error, fileName] = try await getFilename();
if (!ok) {
console.log(filename.toString())
} Should be a problem for TS to solve (which is somewhat a linter os steroids xd) |
yes, there are a lot of ways to shoot yourself ... But with try {
const fileName = getFilename();
fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');
} catch (err) {
console.error(err);
} and this a most expected way to write the code. |
Not necessarily, the main reason for why this proposal was created is for this super common usecase: let fileName;
try {
fileName = getFilename();
} catch (err) {
console.error(err);
}
// do a lot more stuff (and a lot more possible try/catch/finally)
fs.writeFileSync(`__dirname/${fileName}`, 'Hello World'); Where you need more than 1 try catches (so you can distinguish where the error is coming from) and maybe to write more code after the handlings and only then do what you want to do with the original file. I don't think this proposal is aiming to replace try blocks the same way For example, this below code (thanks chatgpt) is very common inside business code: function processData(data: string) {
try {
console.log("Outer try block");
// Simulate a potential outer error
if (!data) throw new Error("Outer Error: No data provided");
try {
console.log("Middle try block");
// Simulate a parsing error
const parsedData = JSON.parse(data);
console.log("Parsed Data:", parsedData);
try {
console.log("Inner try block");
// Simulate a validation error
if (!parsedData.valid) throw new Error("Inner Error: Invalid data");
console.log("Data is valid:", parsedData);
} catch (innerError) {
console.error("Caught in inner catch:", innerError.message);
}
} catch (middleError) {
console.error("Caught in middle catch:", middleError.message);
}
} catch (outerError) {
console.error("Caught in outer catch:", outerError.message);
}
} where either you use multiple nested catches to handle different errors or you only use one and isn't able to distinguish where specifically the error was being thrown. And using this approach gives a much more readable verison: (thanks chatgpt again) function processData2(data: string) {
console.log('First step block');
// Simulate a potential outer error
if (!data) throw new Error('Outer Error: No data provided');
console.log('Middle try block');
const [ok, error, parsedData] = try JSON.parse(data);
if (!ok) {
console.error('Caught in middle catch:', error.message);
return;
}
console.log('Parsed Data:', parsedData);
// Simulate a validation error
const [ok, innerError, valid] = try validateData(parsedData);
if (!valid) {
console.error('Caught in inner catch:', innerError.message);
return;
}
console.log('Data is valid:', parsedData);
} |
@arthurfiorette this still leaves a bad taste in my mouth - I know the I just don't like the idea of adding complexity to design/optimize for (or even cope with) an anti-pattern - remembering the exact order of those 3 return values is going to be the sort of thing I'd have to look up on a daily basis, I think. 😔 I still think it would make more sense to wrap non-Error values in an Error instance. The engine knows at the @DScheglov based on your comment here I think you misunderstood me the second time again - I am not suggesting every error be wrapped. Only non-Error values would be wrapped, in an Error-subtype with a |
the exceptions are expected to be used in the way to get the function processData(data: string) {
try {
validateInput(data);
const parsedData = parseJson(data);
return validateData(parsedData);
} catch (err) {
if (handleDataProcessingError(err)) return false;
throw err;
}
} See in Playground Actually your refactored with console.error('Caught in middle catch:', error.message);
// ^^^^^ 'error' is of type 'unknown'. console.error('Caught in inner catch:', innerError.message);
// ^^^^^^^^^^ 'innerError' is of type 'unknown'. And worning: const [ok, innerError, valid] = try validateData(parsedData);
// ^^ 'ok' is declared but its value is never read. see in Playground In the real word knowning where the error is caught means nothing. More then, trying to conclude any decision basing only on the place of caught could be dangerous -- you examples shows that. TS founds the error! JS doesn't. More then, because we have |
I've never told that you suggested to wrap everything we caught. ) It did @Arlen22 in the comment#2371679490
Let's guess we have some evil framework, where we call a framework api inside of our code, and the framework calls our code.
Yes, it is an anti-pattern, but it is allowed by ES and TS. It is better to find the solution that doesn't break an existing code. |
even the const none = {
toString() {
throw new ReferenceError('Attemt to resolve none as a string')
},
};
console.log(
JSON.parse(none as any)
); So, never, never, and one more time never rely on the place where you caught the exception. There is another entity that allows us to rely on the "place" where it is received. |
@DScheglov it still sounds like you don't understand what I'm suggesting.
Neither of those things would happen. If a non-Error value is thrown, it isn't wrapped at the It would get wrapped at the (Either way, it would be an extremely strange framework or error-handler that only expects and handles non-Error values.) |
I'm not really a fan of the "ok" syntax; it makes the code unnecessarily verbose for handling edge cases. I would prefer using the try expression as a day-to-day feature because it has a more friendly syntax than having to destructure three values. When discussing the order of values, I believe having the result first is more practical. In everyday work, a result that equals I like to compare this feature to promises and async code. An expression could either succeed and return a value or throw an error, similar to how promises work with Also, for example: let result;
try {
result = await fetch("https://api.example.com");
} catch (error) {
return toast("Something went wrong");
}
// Do something with the result This could be simplified to: const [result] = try await fetch("https://api.example.com");
if (!result) return toast("Something went wrong");
// Do something with the result The second solution:
I'm also convinced that the error should be the exact same as the error thrown by the try await clause. The try await should be a simple, transparent JavaScript feature that doesn't alter the behavior or the API of what was thrown. Don't let the feature do more than what it is intended for. Though, I do agree that this code makes JavaScript look a bit like Go! 😄 |
I got you correctly
export const _try: {
(fn: () => never): Failure;
(fn: () => Promise<never>): Promise<Failure>;
<T>(fn: () => Promise<T>): Promise<ResultTuple<T>>;
<T>(fn: () => T): ResultTuple<T>;
} = <T, >(fn: () => T | Promise<T>): any => {
if (typeof fn !== 'function') {
return failure(new TypeError('Parameter fn is not a function', { cause: fn }));
}
try {
const result = fn();
return isPromise(result)
? result.then(success, failure)
: success(result);
} catch (error) {
return failure(error);
}
}; Where the export type Failure = readonly [error: Error];
export type Success<T> = readonly [error: undefined, value: T];
export type ResultTuple<T> = Failure | Success<T>;
const success = <T, >(value: T): Success<T> => [undefined, value] as const;
const failure = (err: unknown): Failure => [TryError.ensureError(err)] as const; And finally the class TryError extends Error {
static ensureError(value: unknown): Error {
return Error.isError(value) ? value : new TryError(value);
}
constructor(public readonly cause: unknown) {
super('Non-Error has been thrown.', { cause })
}
} See Playground
So, the pointed above solution affects already existing code Let's suppose we have some framework, that allows us to write the following code: type UseFn = <T>(fn: () => Promise<T>) => T;
const HelloComponent = (use: UseFn) => {
const name = use(() => Promise.resolve('World'));
return `<h1>${name}</h1>`
} To render the component async function render(
fnComponent: (useFn: UseFn) => string,
) {
let useCounter = 0;
const useValues: unknown[] = [];
function use<T>(fn: () => Promise<T>): T {
if (useValues.length <= useCounter) throw fn();
return useValues[useCounter++] as T;
}
const run = async () => {
try {
useCounter = 0;
return fnComponent(use);
} catch (err) {
if (isPromise(err)) {
useValues[useCounter] = await err;
return run();
}
throw err;
}
}
return run();
} And rendering the Component: render(HelloComponent).then(console.log); Now, let's change the const HelloComponent = (use: UseFn) => {
const [error, name] = _try(
() => use(() => Promise.resolve('World'))
);
if (error) {
console.log('We got an error', error);
throw error;
}
return `<h1>${name}</h1>`
} We will get an error. Just because we are wrapped the Promise with error. Why am I talking about this affects an existing code. } catch (err) {
+ if (err instanceof Error && isPromise(err.cause)) {
+ useValues[useCounter] = await err.cause;
+ return run();
+ }
if (isPromise(err)) { So, despite we are using new syntax only in new code, we have to update an existing as well. |
it looks like you need another kind of expression: const response = try? await fetch(...);
// or
const response = try! await fetch(...);
if (response == null) ... In general, to ignore error -- is an anti-pattern, so it is not a good idea to encourage this by introducing new "concise" syntax. |
@arthurfiorette The example you gave can very easily be rewritten in a cleaner way (since errors can't escape the catch block unless rethrown):
Becomes: function processData(data: string) {
try {
console.log("Outer try block");
// Simulate a potential outer error
if (!data) throw new Error("Outer Error: No data provided");
} catch (outerError) {
console.error("Caught in outer catch:", outerError.message);
}
let parsedData; // the `let` can become `const` if the [write-once const proposal](https://github.com/nzakas/proposal-write-once-const/) is accepted
try {
console.log("Middle try block");
// Simulate a parsing error
parsedData = JSON.parse(data);
console.log("Parsed Data:", parsedData);
} catch (middleError) {
console.error("Caught in middle catch:", middleError.message);
}
try {
console.log("Inner try block");
// Simulate a validation error
if (!parsedData.valid) throw new Error("Inner Error: Invalid data");
console.log("Data is valid:", parsedData);
} catch (innerError) {
console.error("Caught in inner catch:", innerError.message);
}
} Plus the function processData2(data: string) {
console.log('First step block');
// Simulate a potential outer error
if (!data) throw new Error('Outer Error: No data provided');
console.log('Middle try block');
let parsedData; // the `let` can become `const` if the [write-once const proposal](https://github.com/nzakas/proposal-write-once-const/) is accepted
try {
parsedData = JSON.parse(data);
} catch (error) {
console.error('Caught in middle catch:', error.message);
return;
}
console.log('Parsed Data:', parsedData);
// Simulate a validation error
try {
const valid = validateData(parsedData);
if (!valid) throw new Error("Inner Error: Invalid data");
} catch (innerError) {
console.error('Caught in inner catch:', innerError.message);
return;
}
console.log('Data is valid:', parsedData);
} |
In your first snippet, the This is exactly a case for the try expression -- to achieve a "flat" flow without |
@DScheglov Thanks for catching that small oversight, its fixed now 😄 |
@DScheglov And as I mentioned in the code snippet, the Can you explain what you mean by a "flat" flow? |
"Flat" -- without nested "try"-s About the write-once const proposal: it is in the "Stage: -1". |
I'm not arguing against the do-expression proposal nor against this one. The write-once It's still unclear to me what exactly is the problem this proposal is trying to solve (I asked on multiple occasion but got no answer)... |
That’s a great observation. What I’m noticing is that some developers prefer to have a default nullish value in case of an unsuccessful computation. Typically, these developers also favor the data-first tuple approach. The popularity of such approach we can see on how many downloads the nice-try library has: ~13.5M/week: const [data] = try JSON.parse(input); // data undefined or parsed json So, it is definitelly the case, that must be considered for the proposal. The second one is pointed by @arthurfiorette's comment#2374175054:
The most popular lib for that (that I found) is safely-try with ~9k/week downloads. The difference between these two cases: 1.5k times!!! The reason for this difference could be explained by the findings from a survey conducted by the TypeScript team regarding exception usage:
Source: #13219 (commnet) cc: @arthurfiorette UPD: |
I agree that "default nullish value in case of an unsuccessful computation" can be useful in some cases (I have multiple those most projects), but it feels to me like it should always be explicitly clear that the error is being ignored (which this proposal doesn't yet enforce). As for the "get rid of nested BTW, I'm all for the "do"-expression proposal and would love to see it added to JS 😄 |
100%! Joining to this. But the Champion of the proposal is disagree with that even error-first approach const [, data] = try JSON.parse(input); is an implicit error ignoring. So, it looks very debatable issue.
I'm not sure about that. I'd prefer to avoid discussing the "write-once const" here, but if this proposal is implemented, we might end up with one place where the variable is declared and multiple places where it is assigned a value, which could make the code harder to read and understand. In this sense,
I guess it makes sense to extend the "do"-expression proposal with adding a catch block after the do-block: const data = do { Parse.json(input) } catch { null }; It looks much more concise then: const data = do {
try { Parse.json(input) } catch { null }
} |
I also agree that we should avoid discussing other proposals here, but we also can't discuss this proposal in isolation when other proposals might solve the same problems. 😅 But just to clarify, the "write-once const" proposal would only allow detaching the initialization of a |
what do you think about the following approach: const [caught, result] = try mayThrow();
if (caught) {
console.error(caught.error);
throw caught; // the same as: throw caught.error;
}
handleTheResult(result); Where the interface Caught {
readonly error: unknown;
readonly [Symbol.exception]: unknown;
} and the Let The approach covers |
A symbol protocol is imo a nonstarter and not a good idea, especially since a getter for |
actually, we can start without |
@DScheglov to be clearer, i think |
So, do you think a tuple or an object with an explicit discriminator are the only options we have for the try expression return? |
Yes. |
I've been playing with
|
Additional post on twitter
As you might've seen in some issues around here. There's a discussion about the usage of
[tuples]
or{objects}
as the result for this operator, as well asfalsy
errors, likethrow undefined
or more common cases likethrow functionThatMightReturnUndefinedOrAnError()
.Here's things that this proposal will NOT solve for you.
This proposal does not aims to create a
Result<T>
like object class with unary operations like.unwrap()
,.mapError(fn)
and/or.orElse(data)
. This can be easily achievable by current JS code and I'm pretty sure there's tons of libraries like this one out there.This proposal will not change the behavior of any function or its return type. The safe assignment operator (or future try expression as of Discussion: Preferred operator/keyword for safe assignment #4) is to improve the caller experience and not the callee.
Return type syntaxes
Tuple:
Tuples are easier to destructure and follows the concept of error first destructuring.
Simple use case:
Complex use case:
Objects:
Simple use case:
Complex use case:
Both of them
An Object with
error
,data
property andSymbol.iterator
is also a valid solution.Later we would need to improve the TS typings for that Symbol.iterator result to not mix
error | data
types on both destructure values, but since TS already did that for tuples ([1, 2, 3]
) it won't be a problem.Falsy values
Ideally, we should have some sort of wrapper against
falsy
errors, as in the end having falsy values wrapped are much helpful in such problems.Besides intended use cases, no one expects to face a
null
inside acatch(error)
block or neither will know how to handle that correctly.Since errors are now treated as values, in both return types, we should have a way to differentiate them.
We have two main approaches:
helper property
Just like on
Promise.allSettled
'sstatus
property, we could have a.ok
or something to tell the user that if the operation failed or not.Error wrapper
Wrapping the falsy value (
0
,''
,null
, ...) into aNullableError
class or something could arguably be presented as a good feature of this proposal as well, where we reduce the amount or burden that these errors would mean into a more useful value.For example, an application crashing with
undefined
and nothing more in the terminal is way worse than having an error like the following is much more useful in all meanings:The text was updated successfully, but these errors were encountered: