search

jfmengels / node-elm-review

CLI for elm-review
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
BSD 3-Clause "New" or "Revised" License
48 stars 25 forks source link

Concurrency #292

Closed lishaduck closed 3 weeks ago

lishaduck commented 3 weeks ago

Theoretically contains some zalgo, improves some stacktraces. Practically reduces cognitive overhead and consistency with the ecosystem.

Here's the diff as GH doesn't play well with stacks from forks: https://github.com/lishaduck/node-elm-review/compare/sort-imports...async

jfmengels commented 3 weeks ago

improve stacktraces by requiring async code to be async

Question out of curiosity: Can you explain how/why stack traces are improved by this change? I'd love an example of before/after so that I can have a clearer understanding?

lishaduck commented 3 weeks ago

Can you explain how/why stack traces are improved by this change? I'd love an example of before/after so that I can have a clearer understanding?

If you await a promise, it gets added to the stack, if you return it, it doesn't. The return-await rule doesn't trigger in sync code, so by making them async, you fix that. Essentially, all of the footguns from return-await. By requiring functions that return a promise to be async, you a) prevent zalgo (sync rejection/fulfillment from a promise-returning function) and b) encourage awaiting promises over returning them.

As for examples, I guess it'd just be that it's omitted in the list. Like, rather than:

Uncaught Err: [...] at <anonymous>
  theFunctionThatThrew
  (anonymous) @ unknown

You'd get 

Uncaught Err: [...] at <anonymous>
  (anonymous) @ unknown

It'll skip including when it was passed through without an await as being part of the stack. From experience, it's messed me up a few times before.

If you'd like, I could go write some code that throws, but I don't have any real examples on hand.

TSESlint docs:

jfmengels commented 3 weeks ago

Thank your for the great explanation :blush: