Skip to content

Fix issue with catching exceptions thrown by engage #50

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.

Sign up for GitHub

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

Merged
coltfred merged 4 commits into from
Jul 3, 2025
Merged

Fix issue with catching exceptions thrown by engage #50

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72c809c
Initial commit with tests
coltfred Jul 1, 2025
3e5a5fa
Attempt at a fix
coltfred Jul 1, 2025
58e3951
Put comment back on
coltfred Jul 1, 2025
45a3674
Address code review
coltfred Jul 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
coverage
node_modules
yarn-error.log
Future.js
Future.js
.envrc
81 changes: 72 additions & 9 deletions Future.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ describe("Future", () => {
expect(resolveSpy).not.toHaveBeenCalled();
});

test("invokes reject when action throws exception", () => {
Comment thread
coltfred marked this conversation as resolved.
const actionSpy = jasmine.createSpy("futureAction").and.throwError("forced error");
const rejectSpy = jasmine.createSpy("rejectSpy");
const resolveSpy = jasmine.createSpy("resolveSpy");
new Future(actionSpy).engage(rejectSpy, resolveSpy);
// test("invokes reject when action throws exception", () => {
// const actionSpy = jasmine.createSpy("futureAction").and.throwError("forced error");
// const rejectSpy = jasmine.createSpy("rejectSpy");
// const resolveSpy = jasmine.createSpy("resolveSpy");
// new Future(actionSpy).engage(rejectSpy, resolveSpy);

expect(actionSpy).toHaveBeenCalledWith(rejectSpy, resolveSpy);
expect(rejectSpy).toHaveBeenCalledWith(new Error("forced error"));
expect(resolveSpy).not.toHaveBeenCalled();
});
// expect(actionSpy).toHaveBeenCalledWith(rejectSpy, resolveSpy);
// expect(rejectSpy).toHaveBeenCalledWith(new Error("forced error"));
// expect(resolveSpy).not.toHaveBeenCalled();
// });

test("resolves with expected value on success", () => {
const action = (_reject: (e: Error) => void, resolve: (val: string) => void) => {
Expand All @@ -37,6 +37,69 @@ describe("Future", () => {

expect(resolveSpy).toHaveBeenCalledWith("my value");
});
test("does not get run if engages above this scope throw", (done) => {
Comment thread
coltfred marked this conversation as resolved.
Outdated
let mapCalledTimes = 0;
let handleWithCalledTimes = 0;
const action = Future.of(33)
.handleWith((e: Error) => {
// eslint-disable-next-line no-console
console.log(`failed an infallible future: ${e.message}`);
handleWithCalledTimes++;
return Future.of(-1);
})
.map((r) => {
mapCalledTimes++;
return r;
});

try {
action.engage(
(e) => {
throw e;
},
(r) => {
throw new Error(`oh no, something went wrong after the future has run to completion on ${r}`);
}
);
} catch (e) {
expect(handleWithCalledTimes).toBe(0);
expect(mapCalledTimes).toBe(1);
done();
}
});

test("does not get run if engages above this scope throw in the rejection", (done) => {
let mapCalledTimes = 0;
let handleWithCalledTimes = 0;
const action: Future<Error, number> = Future.reject(new Error("error message"))
.handleWith((e): any => {
// eslint-disable-next-line no-console
console.log(`failed a future: ${e.message}`);
handleWithCalledTimes++;
return Future.of(-1);
})
.map((r) => {
// eslint-disable-next-line no-console
console.log(`mapped`);
mapCalledTimes++;
return r;
});

try {
action.engage(
(e) => {
throw e;
},
(r) => {
throw new Error(`oh no, something went wrong after the future has run to completion on ${r}`);
}
);
} catch (e) {
expect(handleWithCalledTimes).toBe(1);
Copy link
Copy Markdown
Member

@skeet70 skeet70 Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand based on the description, where does this test show that it the handleWith wasn't run? It looks like it shows that it does, and it's not clear if it's from its own rejection or the rejection thrown in the engage.

Copy link
Copy Markdown
Member Author

@coltfred coltfred Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the test to show that it's the error that came from the Future.reject

expect(mapCalledTimes).toBe(1);
done();
}
});
});

describe("toPromise", () => {
Expand Down
34 changes: 22 additions & 12 deletions Future.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,18 @@ export default class Future<L, R> {
* @param {Function} resolve Handler if Future fully executed successfully
*/
engage(reject: Reject<L>, resolve: Resolve<R>): void {
try {
//In the case where the action call just completely blows up, prevent against that and invoke reject.
this.action(reject, resolve);
} catch (error: any) {
reject(error);
this.action(reject, resolve);
}

private engageAndCatch(reject: Reject<L>, resolve: Resolve<R>, safe = true): void {
if (safe) {
try {
this.engage(reject, resolve);
} catch (e) {
reject(e as L);
}
} else {
this.engage(reject, resolve);
}
}

Expand All @@ -43,7 +50,7 @@ export default class Future<L, R> {
* @return {Promise<R>} Start execution of the Future but return a Promise which will be resolved/reject when the Future is
*/
toPromise(): Promise<R> {
return new Promise<R>((resolve: Resolve<R>, reject: Reject<L>) => this.engage(reject, resolve));
return new Promise<R>((resolve: Resolve<R>, reject: Reject<L>) => this.engageAndCatch(reject, resolve));
Copy link
Copy Markdown
Member

@skeet70 skeet70 Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be engage? A throw from inside a Promise like this should get caught by the promise. Actually why does engageAndCatch exist with the safe flag like this at all? If we want it, can't we have noThrowEngage or engageAndCatch without the safe argument and always do the safe logic? Anywhere you would write engageAndCatch(x, y, false) you can write engage(x, y) instead right?

Copy link
Copy Markdown
Member Author

@coltfred coltfred Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right on both accounts. Fixed and added a test that shows that an error thrown in the future will be caught by the promise if in a chain.

}

/**
Expand All @@ -60,7 +67,7 @@ export default class Future<L, R> {
*/
flatMap<NewResultType>(next: (data: R) => Future<L, NewResultType>): Future<L, NewResultType> {
return new Future<L, NewResultType>((reject: Reject<L>, resolve: Resolve<NewResultType>) => {
this.engage(reject, (data: R) => next(data).engage(reject, resolve));
this.engageAndCatch(reject, (data: R) => next(data).engageAndCatch(reject, resolve));
});
}

Expand All @@ -72,7 +79,7 @@ export default class Future<L, R> {
handleWith<RepairedType extends R>(errHandler: (e: L) => Future<L, RepairedType>): Future<L, RepairedType> {
return new Future<L, RepairedType>((reject: Reject<L>, resolve: Resolve<RepairedType>) => {
this.engage((error) => {
errHandler(error).engage(reject, resolve);
errHandler(error).engageAndCatch(reject, resolve, false);
}, resolve as Resolve<R>); //Type cast this as the resolved method should be able to handle both R and RepairedType
});
}
Expand All @@ -83,7 +90,7 @@ export default class Future<L, R> {
*/
errorMap<LB>(mapper: (error: L) => LB): Future<LB, R> {
return new Future<LB, R>((reject: Reject<LB>, resolve: Resolve<R>) => {
this.engage((error) => reject(mapper(error)), resolve);
this.engageAndCatch((error) => reject(mapper(error)), resolve);
});
}

Expand All @@ -97,6 +104,7 @@ export default class Future<L, R> {
try {
result = fn();
} catch (e: any) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
return reject(e);
}
resolve(result);
Expand All @@ -114,6 +122,7 @@ export default class Future<L, R> {
try {
promiseResult = fn();
} catch (e: any) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
return reject(e);
}
//We have to support both Promise and PromiseLike methods as input here, but treat them all as normal Promises when executing them
Expand Down Expand Up @@ -151,6 +160,7 @@ export default class Future<L, R> {
try {
result = fn(a);
} catch (e: any) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
return reject(e);
}
resolve(result);
Expand All @@ -169,7 +179,7 @@ export default class Future<L, R> {
let count = 0;
let done = false;

future1.engage(
future1.engageAndCatch(
(error) => {
if (!done) {
done = true;
Expand All @@ -184,7 +194,7 @@ export default class Future<L, R> {
}
);

future2.engage(
future2.engageAndCatch(
(error) => {
if (!done) {
done = true;
Expand Down Expand Up @@ -249,7 +259,7 @@ export default class Future<L, R> {
}

futures.forEach((futureInstance, index) => {
futureInstance.engage(
futureInstance.engageAndCatch(
(error) => {
reject(error);
},
Expand Down
57 changes: 57 additions & 0 deletions flake.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
description = "FutureJS";
inputs.flake-utils.url = "github:numtide/flake-utils";

outputs = { self, nixpkgs, flake-utils }:
flake-utils.lib.eachDefaultSystem
(system:
let
pkgs = nixpkgs.legacyPackages.${system};
in
{
devShell = pkgs.mkShell
{
buildInputs =
[
pkgs.nodejs_20
(pkgs.yarn.override {
nodejs = pkgs.nodejs_20;
})
];
};
});
}
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
},
"devDependencies": {
"@types/jest": "^26.0.22",
"@typescript-eslint/eslint-plugin": "^4.21.0",
"@typescript-eslint/parser": "^4.21.0",
"@typescript-eslint/eslint-plugin": "^5.59.11",
"@typescript-eslint/parser": "^5.59.11",
"eslint": "^7.23.0",
"eslint-plugin-import": "^2.22.1",
"eslint-plugin-jsdoc": "^32.3.0",
Expand Down
Loading