Catch exceptions #224
Catch exceptions #224
Conversation
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 48 48
Lines 1046 1143 +97
=====================================
+ Hits 1046 1143 +97
Continue to review full report at Codecov.
|
|
The benchmark results actually look quite different in Node v8. I was using v9 before. |
|
Given: const unsafe = Math.random() > 0.5 ? {foo: 1} : null
const noop = () => {}I'm a bit concerned that the following: Future((rej, res) => {
const foo = unsafe.foo
res(foo)
})._interpret(noop, noop, noop)Will not be equal to: const foo = unsafe.foo
Future((rej, res) => {
res(foo)
})._interpret(noop, noop, noop)Essentially the exception catching feature removes a level of being able to reason about, and safely refactor, certain code. |
|
@Avaq I would not have considered these to be equivalent either way. Especially when considering potential failures. Should On the other hand, if this |
|
|
||
| if(typeof Object.create !== 'function') error('Please polyfill Object.create to use Fluture'); | ||
| if(typeof Object.assign !== 'function') error('Please polyfill Object.assign to use Fluture'); | ||
| if(typeof Array.isArray !== 'function') error('Please polyfill Array.isArray to use Fluture'); |
There was a problem hiding this comment.
This chunk was useless, because rollup changes the order of code evaluation in such a way that this code is reached only after some parts have already tried to use these functions. It's also not super important, as it only served to slightly improve the error messages.
| @@ -20,7 +14,7 @@ Future.map = map; | |||
| Future.bimap = bimap; | |||
| Future.chain = chain; | |||
|
|
|||
| var Par = concurrify(Future, never, race, parallelAp); | |||
| var Par = concurrify(Future, never, race, function parallelAp(a, b){ return b._parallelAp(a) }); | |||
There was a problem hiding this comment.
I changed parallelAp from a static constructor to a standard Action. This saved me writing some extra code.
| "test:build": "npm run clean && npm run build && mocha --require @std/esm --ui bdd --reporter dot --bail test/**.buildtest.js", | ||
| "test:coverage": "npm run clean && nyc --include src mocha --require @std/esm --ui bdd --reporter dot test/**.test.js || true", | ||
| "coverage:upload": "nyc report --reporter=text-lcov > coverage.lcov && codecov", | ||
| "coverage:report": "nyc report --reporter=html", |
There was a problem hiding this comment.
These changes facilitate potentially failing tests while testing for coverage -- annoyingly when testing toString, sometimes the coverage instrumentation breaks expected output.
|
I just flipped the libraries I'm comparing, so |
|
Seeing as the diff is so huge, I would like to highlight some of the important changes and decisions, so that they can be discussed.
|
It's kinda hard for me to judge because I wouldn't wrap big chunks of computation into a Also, I'm guessing that the solution involves a rethrowing an exception. In this case, a debugger won't pause on the original exception, and users won't be able to inspect a frozen program. I, for one, rely a lot on this feature of a debugger.
A possible solution would be to direct bugs into promises' failure path and direct both errors and normal results into promises' success, using an improvised Either. Basically what fun-task does https://github.com/rpominov/fun-task/blob/master/docs/api-reference.md#tasktopromiseoptions 😅 |
Generally this kind of thing happens if there is some underlying abstraction calling your code, such as when the Future is forked as part of an Express middleware.
I like this idea, but I don't like introducing a competing Either type. Perhaps I can change the signature to: promise :: (a -> c) -> (b -> c) -> Future a b -> Promise cOne would use it like: const toPromise = Future.promise(S.Left, S.Right);
toPromise(Future.reject('meh')) // Promise<Left('meh')> |
This looks great 👍 Regarding rethrowing an exception on a next tick, I still think it's a bad idea. Maybe it's just me, but I can get very angry at times when a library messes with exceptions somehow and I need to fix a bug asap. |
My reasoning for this behaviour was that I wanted to make the behaviour more consistent. Nine out of ten times when you However, I think it's a very compelling point that this is kind of anti-tooling. It's probably easier to track an error that was thrown, caught, and retrown all in the same tick, using various debuggers. I also like the idea that So in conclusion, I will experiment with synchronously thrown errors, and likely amend this PR. Thank you for your feedback Roman! :D |
I'm going to leave this for another PR, leaving the signature (and behaviour) of |
Breaking changes - #224 Exceptions are now caught and rethrown in `fork`. - #230 Many of the TypeScript type definitions that used `never` now use a generic instead. - #238 When unsubscribing from a Future created by `hook`, the cancellation signal is no longer sent to the disposal Future. - #266 Exported TypeScript interfaces have been renamed to avoid naming conflicts with exported values. New features - #250 Included interoperability with Sanctuary Show - #261 Added a new function, `forkCatch`, which allows for exception recovery.
When a Future created from a Promise would enter crashed state later along a synchronous pipeline, the original Promise would catch the error and change its state to rejected. That rejection would never be handled, and cause the Promise to swallow it, triggering the unhandledRejection events in some Promise implementations. Related to #150 and #224.
When a Future created from a Promise would enter crashed state later along a synchronous pipeline, the original Promise would catch the error and change its state to rejected. That rejection would never be handled, and cause the Promise to swallow it, triggering the unhandledRejection events in some Promise implementations. Related to #150 and #224.
When a Future created from a Promise would enter crashed state later along a synchronous pipeline, the original Promise would catch the error and change its state to rejected. That rejection would never be handled, and cause the Promise to swallow it, triggering the unhandledRejection events in some Promise implementations. Related to #150 and #224.
So I finally reached a point where all the original unit tests are passing again. This marks an important milestone in my work towards implementing a solution for #153.
But the suspicion I mentioned having in #196 has been confirmed: These changes appear to affect performance significantly. In particular the creation of Futures has slowed down, in many cases to about half of the speed, in some even to 10% of the original speed. I don't yet understand why this happens.
Additional things to do:
forkis not used in tests asserting_interpret- also rename test cases@@typeproperty is bumped/cc @Beanow