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

This doesn't accomplish anything #35

Open
alray2569 opened this issue Aug 26, 2024 · 19 comments
Open

This doesn't accomplish anything #35

alray2569 opened this issue Aug 26, 2024 · 19 comments

Comments

@alray2569
Copy link

alray2569 commented Aug 26, 2024

I won't beat around the bush: I don't like this proposal. I don't think it achieves any of its stated goals. Let's go through them:

Simplified Error Handling

This is the weakest motivation listed because it provides no explanation of why try-catch blocks are more complicated than "safe assignment," or what exactly safe assignment is supposed to simplify. As far as I'm concerned, this is purely a matter of opinion.

Enhanced Readability

Compare the following code: snippets

try {
    const value = codeThatMightThrow();
    // code that depends on value...
} catch e {
    handleError(e);
}
const [e, value] ?= codeThatMightThrow();
if (e) {
    handleError(e);
} else {
    // code that depends on value...
}

In the first one, all of our application logic is grouped together into the try block, and the error handling is grouped together into the catch block. In the second example, the error handling code interrupts the application logic. By splitting apart logical contexts, we have made the code less readable.

Especially ironic is the claim that safe assignment will reduce nesting. Consider your example code:

async function getData() {
    const response = await fetch("https://api.example.com/data");
    const json = await response.json();
    return validationSchema.parse(json);
}

The most logical error handling in this case is to log the error and return null. And despite much hand-wringing about different types of errors, we don't actually care what error happened because either we got our data or we didn't. So let us fix this code with try-catch:

async function getData() {
    try {
        const response = await fetch("https://api.example.com/data");
        const json = await response.json();
        return validationSchema.parse(json);
    } catch e {
        console.log(`Error getting data: ${e}`);
        return null;
    }
}

Let us now fix the same code with safe assignment (and without the heavyhanded multiple returns in the example solution, since they are both inconsistent with the function contract and introduce flow complexity):

async function getData() {
    let toReturn = null;
    const [fetchError, response] ?= await fetch("https://api.example.com/data");
    if (!fetchError) {
        const [jsonError, json] ?= await response.json();
        if (!jsonError) {
            const [parseError, parsed] ?= validationSchema.parse(json);
            if (!parseError) {
                toReturn = parsed;
            } else {
                console.log(`Error getting data: ${parseError}`);
            }
        } else {
            console.log(`Error getting data: ${jsonError}`);
        }
    } else {
        console.log(`Error getting data: ${fetchError}`);
    }
    return toReturn;
}

This could hypothetically be mitigated by some sort of safe-coalescing operator, say, ?&. (For people who are interested the gritty theoretical details, this would turn the safe assignment pattern into a monad with unit = value => [null, value] and bind = (a, b) => a ?& b.) In practice, this would look like this:

async function getData() {
    const [error, parsed] ?= await fetch("https://api.example.com/data")
        ?& async response => await response.json()
        ?& json => validationSchema.parse(json);
    if (error) {
        console.log(`Error getting data: ${jsonError}`);
        return null;
    } else {
        return parsed;
    }
}

If we also added a safe-fallback operator, say, ?|, we could specify a default value inline, for example:

async function getData() {
    return await fetch("https://api.example.com/data")
        ?& async response => await response.json()
        ?& json => validationSchema.parse(json)
        ?| error => (console.log(`Error getting data: ${jsonError}`), null);
}

Now we're actually getting into the realm of being more readable than try-catch. (By the way, if this code looks strangely familiar, that's because it's basically the same thing as .then/.catch for promises, but with a uniform operator instead of a bespoke set of methods.

BUT — ?& and ?| pretty much obviate ?=. We can use the former pair to chain and handle errors before we even make an assignment. So this is really starting to look like an alternative proposal now. You'll notice that ?= is gone from my example.

Consistency Across APIs

Ah, yes, a classic: Let us achieve consistency across APIs by giving API developers yet another way to be inconsistent. Need I say more?

Improved Security

This only improves security if we break every existing API to enforce the use of safe assignment. As long as I can forget to use safe assignment, it's not safe. And of course, if we rewrite things like async/await and parsing to enforce safe assignment, we break decades of code. Certainly, this could be enforced by a linter, but so could try-catch.

Do you know what is enforceable? Just returning an error-value tuple outright. And that doesn't require any new magical syntax, either. Of course, all other arguments above still stand for returning a tuple.

@khaosdoctor
Copy link

Valid arguments, a good write up, but I disagree on a few things.

  • Simplified error handling is indeed a matter of opinion, one which we don't have anywhere to run because the only way to handle errors in JS is with try or .catch.

The nested code will probably not be written like this, just look at how they do it in Go... I'd do it like this:

async function getData() {
    const [fetchError, response] ?= await fetch("https://api.example.com/data");
	if (fetchError) return console.log(`Error getting data: ${fetchError}`);

	const [jsonError, json] ?= await response.json();
	if (jsonError) return console.log(`Error getting data: ${jsonError}`);

	const [parseError, parsed] ?= validationSchema.parse(json);
	if (parseError) return console.log(`Error getting data: ${parseError}`);

	return parsed
}

else is (most of the time) an useless construct of any language.

Now this:

async function getData() {
    return await fetch("https://api.example.com/data")
        ?& async response => await response.json()
        ?& json => validationSchema.parse(json)
        ?| error => (console.log(`Error getting data: ${jsonError}`), null);
}

Is elegant, I'd love to have it as pattern matching in JS, but it is unreadable.

Consistency Across APIs

JavaScript is not, was not, and will probably never be consistent. I talked about this on #24:

  • setTimeout uses callbacks, along with all the timers
  • XHRHttpRequest is event-based, whereas fetch is promise-based
  • All JS events are (you guessed), event-based
  • Async iterators do not return a callback, which should be the consistent return, right? Since setTimeout is there since before them. But they return promises
  • You can handle errors in promises with .then as well as with .catch the difference is the second parameter of then, when null, is ignored
  • Async/await is the exact same thing as .then/catch, the only difference is the nesting level, which is exactly the same proposal as this one
  • You probably saw all the JS memes about how inconsistent it is I guess...

My point is, there's no point on trying to unify and make the library consistent now, 29 years later, when we can't deprecate any old function. If that was the case, parseInt would've been out for ages and jQuery would've been forgotten.

Do you know what is enforceable? Just returning an error-value tuple outright. And that doesn't require any new magical syntax, either. Of course, all other arguments above still stand for returning a tuple.

But that would require a MASSIVE change to all the previous APIs that you just said cannot be broken. I'd love that JS had Go-like (or even Python's) multiple returns with enforced error handling, but it can't, it simply is not possible not because of the technical side, but because of the operational side.

@alray2569
Copy link
Author

alray2569 commented Aug 26, 2024

Ah, see, not I'm not a fan of conditional early returns because I think they make functions harder to understand quickly, and in that regard, else is very often useful. I'm also not a fan of explicitly returning the output of a void function call, so I would object to return console.log(...). Finally, I note even still in your multiple-return example, you've written essentially the same line of code three times.

The hypothetical ?& and ?| operators are a generalized version of .then and .catch, so you can think of that block as something like this:

async function getData() {
    return await fetch("https://api.example.com/data")
        .then(async response => await response.json())
        .then(json => validationSchema.parse(json))
        .catch(error => (console.log(`Error getting data: ${jsonError}`), null));
}

Obviously, that code doesn't actually work, but that's the idea. You could also think of it like the | and || operators in a shell. In a pseudo-JavaScript-shell, you might think of it like this

fetch "https://api.example/data" \
    | response => toJSON response \
    | json => parseSchema validationSchema json \
    || echo "Error getting data: $?"

This also behaves similarly to Haskell's Either. In Haskell:

fetch "https://api.example.com/data" 
    >>= (\response -> toJSON response) 
    >>= (\json -> parseSchema validationSchema json)
    `fromRight` print "Error getting data"

@khaosdoctor
Copy link

khaosdoctor commented Aug 27, 2024 via email

@alray2569
Copy link
Author

Ah, yeah, I don't like that example with the nested conditionals, either. That was the point. You're going to have a hard time convincing me that the most readable version of all of the examples isn't the one that uses try-catch.

@khaosdoctor
Copy link

khaosdoctor commented Aug 27, 2024 via email

@DScheglov
Copy link

@khaosdoctor

But that’s the point. If you don’t like it you don’t have to use it.

Unfortunately we are working not in the vacuum, the option to use ?= will require an additional effort, either:

  • linting setup (by the way, the correspondent rule must be added)
  • code review

So, unfortunatelly, it is not just my choice. It could be a team choice, but the temptation is too high.

@figloalds
Copy link

figloalds commented Aug 28, 2024

I'd say that both things have value in the programming space depending on how the author intends it
But a large try block with multiple points of failure is not as useful as you seem to believe it is
The intentionality behind a code like this:

async function getData() {
    const [fetchError, response] ?= await fetch("https://api.example.com/data");
	if (fetchError) return console.log(`Error getting data: ${fetchError}`);

	const [jsonError, json] ?= await response.json();
	if (jsonError) return console.log(`Error getting data: ${jsonError}`);

	const [parseError, parsed] ?= validationSchema.parse(json);
	if (parseError) return console.log(`Error getting data: ${parseError}`);

	return parsed
}

Is extremely powerful and significantly easier to debug than a ginormous function with one single catch expression.

My concern about this feature is this, suppose we have a code like this

const [err, result] = await fetch(`/api/people/${selectedPerson.id}`);

But selectedPerson is somehow undefined?

Will this syntax wrap the entire right-side expression in error handling or will it behave weird and throw? And what is the actual expectation for this scenario?

@alray2569
Copy link
Author

alray2569 commented Aug 28, 2024

@figloalds I assume you meant:

const [err, result] ?= await fetch(`/api/people/${selectedPerson.id}`);

The proposal states that the ?= operator "is compatible with promises, async functions, and any value that implements the Symbol.result method." This would suggest that your example would continue to throw an error as the error happens before a value is returned to the ?= operator.

Of course, anyone who is paying attention will also recognize that this is error, because something like await fetch("https://arthur.place") doesn't actually produce any of those types, but rather a Response object. Following the normal resolution of operators, the await operator would resolve before ?= and throw an UnhandledPromiseRejection before ?= has a chance to deal with it.

So the text of the proposal is inconsistent with its code examples. Looking at the polyfill raises yet further concerns. The preprocessing is inconsistent, and in fact, the following lines are preprocessed to the same code:

const [error, value] ?= something;
const [error, value] ?= something();

// both yield
const [error, value] = something[Symbol.result]();

This is obviously concerning, but let us set that aside for the moment. How does it treat await? Well, it's also inconsistent, since the ?= preprocessor skips over the await to inspect the function or Promise directly before resolving the await operation like so:

const [error, value] ?= await something;
const [error, value] ?= await something();

// both yield
const [error, value] = await something[Symbol.result]();

This implies that the treatment of the await operator is specially magical and that your example will indeed still throw unless some additional magic is added.

@DScheglov
Copy link

DScheglov commented Aug 28, 2024

@alray2569

If I understand correctly, the syntax couldn't be polifilled at all.

Regarding the ?= operator, I consider it syntactic sugar for:

<left-hand-expr> ?= <right-hand-expr>:

let __temp_result_tuple__N;

try {
  __temp_result_tuple__N = [undefined, <right-hand-expr>]
} catch (error) {
  __temp_result_tuple__N = [
    error ?? new ReferenceError(`Nullish value has been thrown: ${error}`),
    undefined
  ]
}

<left-hand-expr> = __temp_result__N;

But the proposal involves misusing the Symbol.result, which confuses how it works
with any kind of composition:

const div = (a, b) => {
  if (b === 0) throw new Error('Div by zero');
  return a / b;
}

const [err, value] ?= div(1, 0) + div(1, 0);

I cannot understand if it is equal to:

const [err, value] = div[Symbol.result](1, 0) + div[Symbol.result](1, 0);

... I'm really confused with this polifill.

Perhaps, @arthurfiorette can clarify ...

@alray2569
Copy link
Author

alray2569 commented Aug 28, 2024

@DScheglov That's pretty much exactly my hangup, too. If it was just syntactic sugar for try-catch and had the same semantics as try-catch, that would make it much less confusing, but that doesn't seem to be the proposal as it's currently written. As written, there are three special cases:

  • RH is an awaited Promise
  • RH is an awaited async function call
  • RH is an object with a [Symbol.result] method

All three of these cases are handled slightly differently by the proposal, which would create a parsing nightmare (#38), and all uses that don't match one of these three cases throw a TypeError... which is an interesting choice for something that's supposed to reduce the need for try-catch.

When you put those things together, you get seemingly absurd results (#39), most poignant in the following example, where the first of these lines is fine, but the second throws a TypeError even though the RH values by themselves have identical parse trees and behavior:

const [e1, v1] ?= await (Promise.resolve(4)); // ok
const [e2, v2] ?= await Promise.resolve(4);   // TypeError

Plus, these three lines would all behave differently:

const [e1, v1] ?= await fun();   // => await fun[Symbol.result]()
const [e2, v2] ?= await (fun()); // => await fun()[Symbol.result]()
const [e3, v3] ?= (await fun()); // => (await fun())[Symbol.result]()

@JamesRamm
Copy link

The hypothetical ?& and ?| operators are a generalized version of .then and .catch, so you can think of that block as something like this:

async function getData() {
    return await fetch("https://api.example.com/data")
        .then(async response => await response.json())
        .then(json => validationSchema.parse(json))
        .catch(error => (console.log(`Error getting data: ${jsonError}`), null));
}

Obviously, that code doesn't actually work, but that's the idea. You could also think of it like the | and || operators in a shell. In a pseudo-JavaScript-shell, you might think of it like this

Actually, this code pretty much works. You don't need to wrap it all in async/await

function getData() {
     return fetch("https://api.example.com/data")
         .then(response => response.json())
         .then(json => validationSchema.parse(json))
         .catch(error => console.log(`Error getting data: ${jsonError}`));
}

Don't forget that async/await is just syntactic sugar on top of the original Promise API, which is superior IMO anyway and has further useful stuff like all, allSettled, withResolvers...

My opinion was that the Promise API was a really good start on a nice fluent api and instead of improving it further, async/await was tacked on to the language, muddying the waters and resulting in confused proposals like this one....

@DScheglov
Copy link

@JamesRamm ,

Don't forget that async/await is just syntactic sugar on top of the original Promise API, which is superior IMO anyway and has further useful stuff like all, allSettled, withResolvers...

actually the async/await was a syntactic sugar only with babel ... then it became a language feature, and the code written with .then has an absolutely different stack, then code written with async/await.

My opinion was that the Promise API was a really good start on a nice fluent api and instead of improving it further, async/await was tacked on to the language, muddying the waters and resulting in confused proposals like this on

I don't see any relation between the async/await and this proposal. More then, in some meaning this proposal is counter to the async/await. The last one is about moving declarative .then-based code to the imperative code, keeping the monadic nature of Promises (yes, Promise doesn't follow Haskel's contracts, but it is still possible to define Monad with Promise), this approach takes imperative version and tries to transform it to the declarative one, moving the error to the main control flow and breaking the Monadic nature of exceptions (yes, it is also possible to define the Monad with exceptions in terms of Category Theory)

@khaosdoctor
Copy link

@DScheglov That's pretty much exactly my hangup, too. If it was just syntactic sugar for try-catch and had the same semantics as try-catch, that would make it much less confusing, but that doesn't seem to be the proposal as it's currently written. As written, there are three special cases:

  • RH is an awaited Promise
  • RH is an awaited async function call
  • RH is an object with a [Symbol.result] method

All three of these cases are handled slightly differently by the proposal, which would create a parsing nightmare (#38), and all uses that don't match one of these three cases throw a TypeError... which is an interesting choice for something that's supposed to reduce the need for try-catch.

When you put those things together, you get seemingly absurd results (#39), most poignant in the following example, where the first of these lines is fine, but the second throws a TypeError even though the RH values by themselves have identical parse trees and behavior:

const [e1, v1] ?= await (Promise.resolve(4)); // ok
const [e2, v2] ?= await Promise.resolve(4);   // TypeError

Plus, these three lines would all behave differently:

const [e1, v1] ?= await fun();   // => await fun[Symbol.result]()
const [e2, v2] ?= await (fun()); // => await fun()[Symbol.result]()
const [e3, v3] ?= (await fun()); // => (await fun())[Symbol.result]()

I think the written proposal is outdated tbh, people have been chatting about it on #5 #4 and others, apparently the reached conclusion was to remove ?= over try <exp>

@ljharb
Copy link

ljharb commented Sep 4, 2024

@DScheglov async/await is indeed almost entirely syntactic sugar, and is not a complete replacement for Promises. The stack is irrelevant since that’s not part of the language (yet).

@khaosdoctor
Copy link

@JamesRamm ,

Don't forget that async/await is just syntactic sugar on top of the original Promise API, which is superior IMO anyway and has further useful stuff like all, allSettled, withResolvers...

actually the async/await was a syntactic sugar only with babel ... then it became a language feature, and the code written with .then has an absolutely different stack, then code written with async/await.

My opinion was that the Promise API was a really good start on a nice fluent api and instead of improving it further, async/await was tacked on to the language, muddying the waters and resulting in confused proposals like this on

I don't see any relation between the async/await and this proposal. More then, in some meaning this proposal is counter to the async/await. The last one is about moving declarative .then-based code to the imperative code, keeping the monadic nature of Promises (yes, Promise doesn't follow Haskel's contracts, but it is still possible to define Monad with Promise), this approach takes imperative version and tries to transform it to the declarative one, moving the error to the main control flow and breaking the Monadic nature of exceptions (yes, it is also possible to define the Monad with exceptions in terms of Category Theory)

I'll have to agree in parts, I do agree that the proposals follow different ideas of changing the flow order, but in essence, the goal is also to simplify the written part of the language (callback hells, promise hells etc)

@DScheglov
Copy link

DScheglov commented Sep 4, 2024

@DScheglov async/await is indeed almost entirely syntactic sugar, and is not a complete replacement for Promises. The stack is irrelevant since that’s not part of the language (yet).

The first of all noone points that async/await is a replacement for promises. It is obvious that async/await utilise promises.

The second, why is a stack irrelevant?? And why is it important to be a part of language to be relevant??? From the practical point of view it is relevant, especially when we are talking about error handling.

The third, let's talk about what the syntactic sugar is. If it is processed by compiler (including the jit one) in the same way. So, if it is a syntactic sugar or not depends on the compiler or interpeter. As instance for the V8, the async/await is not a syntactic sugar for the promises: https://v8.dev/blog/fast-async

And finally the language feature from the Language point of view could be considered as a syntactic sugar if and only if the language specification directly requires the language feature to be implemented exactly as a syntactic sugar

@DScheglov
Copy link

DScheglov commented Sep 4, 2024

@khaosdoctor

I think the written proposal is outdated tbh, people have been chatting about it on #5 #4 and others, apparently the reached conclusion was to remove ?= over try <exp>

Just replace in the examples ?= with try <expr> and you will get the same concerns -- the behaviour of the "safe assignment" operator is not concistent.

@alexey-sh
Copy link

This is how I usually do

const GET_DATA_STATUS = Object.freeze({
  ok: 'ok',
  invalidData: 'invalidData',
  unauthorized: 'unauthorized'
});
class FetchError extends Error {}
class ParseError extends Error {}

async function getData() {
  const response = await fetch(
    "https://api.example.com/data"
  ).catch(e => throw new FetchError('Failed to fetch data from example com', { cause: e }));
  const json = await response.json().catch(e => throw new ParseError('Failed to parse data from example com', { cause: e }));

  if (json.unauthorized) {
    return { status: GET_DATA_STATUS.unauthorized };
  }
  const validationErrors = validationSchema.parse(json)
  if (validationErrors) {
    return { status: GET_DATA_STATUS.invalidData, errors: validationErrors };
  }
  return { status: GET_DATA_STATUS.ok, data: json };
}

// assume we are in browser, so the comments are about UI
async function main() {
  try {
    const data = await getData();
    if (data.status === GET_DATA_STATUS.unauthorized) {
      // clear auth, go to login page
    } else if (data.status === GET_DATA_STATUS.invalidData) {
      // show message about it
    } else {
       // success, show the data
    }
  } catch (e) {
    if (e instanceOf FetchError) {
      // handle fetch
    } else if (e instanceOf ParseError) {
       // handle converting to json 
    } else {
       // handle unknown
    }
  }
}

@DScheglov
Copy link

@alexey-sh

It’s a bit strange to access body fields before validating the body structure. ;)

What if resonse.json() resolves with null?

In general, yes — let's throw exceptions for unexpected/unrecoverable cases and use error codes for expected/recoverable ones.

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

No branches or pull requests

8 participants
@ljharb @alexey-sh @JamesRamm @khaosdoctor @alray2569 @DScheglov @figloalds and others