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.

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

fix(std): Converting Deferred interface to class #8601

Closed
wants to merge 6 commits into from
Closed

fix(std): Converting Deferred interface to class #8601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3445765
Converting Deferred interface to class
mayankagarwals Dec 3, 2020
916efaa
Fixed typescript errors, added tests
mayankagarwals Dec 3, 2020
05489a3
changed dependent code in std/http/server.ts
mayankagarwals Dec 3, 2020
2d34857
Merge remote-tracking branch 'upstream/master' into deferred_class
mayankagarwals Dec 4, 2020
058ed87
Changed depenedent cli code
mayankagarwals Dec 4, 2020
239516d
fixed lint errors
mayankagarwals Dec 4, 2020
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
10 changes: 5 additions & 5 deletions cli/tests/unit/net_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
assertNotEquals,
assertThrows,
assertThrowsAsync,
deferred,
Deferred,
unitTest,
} from "./test_util.ts";

Expand Down Expand Up @@ -338,7 +338,7 @@ unitTest(
unitTest(
{ perms: { net: true } },
async function netTcpListenIteratorBreakClosesResource(): Promise<void> {
const promise = deferred();
const promise = new Deferred<void>();
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to put the <void> in here?

Copy link
Contributor Author

@mayankagarwals mayankagarwals Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, or else the default type is {} and mandates a parameter of type {} when resolve is called. This fix was introduced in a later version of typescript though (microsoft/TypeScript#39817).
Edit: This was introduced in the latest version of typescript released last month.

Copy link
Member

Choose a reason for hiding this comment

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

#7573 (comment)

Copy link
Member

@ry ry Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Writing new Deferred<void> is fairly obnoxious. Much longer than deferred. We should reconsider this change.

Copy link
Collaborator

@nayeemrmn nayeemrmn Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ry Additionally, extending Promise has some annoying behaviours which forces it to accept an executor function, which deferred() did not. It makes .then() calls also return Deferred instances which makes no sense.

Copy link
Member

@bartlomieju bartlomieju Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm actually pretty happy with current implementation, I think we should only change Object.assign() bit to directly assign resolve and reject to promise

Copy link
Contributor Author

@mayankagarwals mayankagarwals Dec 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ry @nayeemrmn, I think we'll need to explicitly write deferred<void> too. Not mentioning correct type in https://github.com/denoland/deno/blob/master/std/async/deferred.ts#L8 allows current behaviour but has the downside of not following latest promise structure. But that can be fixed by adding right type to current implementation too.

I don't mind the current implementation either but addition to @bartlomieju comment, I think we should consider following typescript's promise structure


async function iterate(listener: Deno.Listener): Promise<void> {
let i = 0;
Expand Down Expand Up @@ -472,7 +472,7 @@ unitTest(
async function netCloseWriteSuccess() {
const addr = { hostname: "127.0.0.1", port: 3500 };
const listener = Deno.listen(addr);
const closeDeferred = deferred();
const closeDeferred = new Deferred<void>();
listener.accept().then(async (conn) => {
await conn.write(new Uint8Array([1, 2, 3]));
await closeDeferred;
Expand Down Expand Up @@ -506,7 +506,7 @@ unitTest(
async function netDoubleCloseWrite() {
const addr = { hostname: "127.0.0.1", port: 3500 };
const listener = Deno.listen(addr);
const closeDeferred = deferred();
const closeDeferred = new Deferred<void>();
listener.accept().then(async (conn) => {
await closeDeferred;
conn.close();
Expand All @@ -529,7 +529,7 @@ unitTest(
},
async function netHangsOnClose() {
let acceptedConn: Deno.Conn;
const resolvable = deferred();
const resolvable = new Deferred<void>();

async function iteratorReq(listener: Deno.Listener): Promise<void> {
const p = new Uint8Array(10);
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/unit/performance_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import {
assert,
assertEquals,
assertThrows,
deferred,
Deferred,
unitTest,
} from "./test_util.ts";

unitTest({ perms: { hrtime: false } }, async function performanceNow(): Promise<
void
> {
const resolvable = deferred();
const resolvable = new Deferred<void>();
const start = performance.now();
setTimeout((): void => {
const end = performance.now();
Expand Down
6 changes: 3 additions & 3 deletions cli/tests/unit/signal_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
assert,
assertEquals,
assertThrows,
deferred,
Deferred,
unitTest,
} from "./test_util.ts";

Expand Down Expand Up @@ -106,7 +106,7 @@ unitTest(
unitTest(
{ ignore: Deno.build.os === "windows", perms: { run: true, net: true } },
async function signalStreamTest(): Promise<void> {
const resolvable = deferred();
const resolvable = new Deferred<void>();
// This prevents the program from exiting.
const t = setInterval(() => {}, 1000);

Expand Down Expand Up @@ -137,7 +137,7 @@ unitTest(
unitTest(
{ ignore: Deno.build.os === "windows", perms: { run: true } },
async function signalPromiseTest(): Promise<void> {
const resolvable = deferred();
const resolvable = new Deferred<void>();
// This prevents the program from exiting.
const t = setInterval(() => {}, 1000);

Expand Down
2 changes: 1 addition & 1 deletion cli/tests/unit/test_util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export {
fail,
unreachable,
} from "../../../std/testing/asserts.ts";
export { deferred } from "../../../std/async/deferred.ts";
export { Deferred } from "../../../std/async/deferred.ts";
export { readLines } from "../../../std/io/bufio.ts";
export { parse as parseArgs } from "../../../std/flags/mod.ts";

Expand Down
20 changes: 10 additions & 10 deletions cli/tests/unit/timers_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
assert,
assertEquals,
assertNotEquals,
deferred,
Deferred,
unitTest,
} from "./test_util.ts";

Expand All @@ -12,7 +12,7 @@ function waitForMs(ms: number): Promise<number> {
}

unitTest(async function timeoutSuccess(): Promise<void> {
const promise = deferred();
const promise = new Deferred<void>();
let count = 0;
setTimeout((): void => {
count++;
Expand All @@ -24,7 +24,7 @@ unitTest(async function timeoutSuccess(): Promise<void> {
});

unitTest(async function timeoutArgs(): Promise<void> {
const promise = deferred();
const promise = new Deferred<void>();
const arg = 1;
setTimeout(
(a, b, c): void => {
Expand Down Expand Up @@ -79,7 +79,7 @@ unitTest(async function timeoutCancelMultiple(): Promise<void> {

unitTest(async function timeoutCancelInvalidSilentFail(): Promise<void> {
// Expect no panic
const promise = deferred();
const promise = new Deferred<void>();
let count = 0;
const id = setTimeout((): void => {
count++;
Expand All @@ -95,7 +95,7 @@ unitTest(async function timeoutCancelInvalidSilentFail(): Promise<void> {
});

unitTest(async function intervalSuccess(): Promise<void> {
const promise = deferred();
const promise = new Deferred<void>();
let count = 0;
const id = setInterval((): void => {
count++;
Expand Down Expand Up @@ -155,7 +155,7 @@ unitTest(async function fireCallbackImmediatelyWhenDelayOverMaxValue(): Promise<
});

unitTest(async function timeoutCallbackThis(): Promise<void> {
const promise = deferred();
const promise = new Deferred<void>();
const obj = {
foo(): void {
assertEquals(this, window);
Expand All @@ -182,7 +182,7 @@ unitTest(async function timeoutBindThis(): Promise<void> {
];

for (const thisArg of thisCheckPassed) {
const resolvable = deferred();
const resolvable = new Deferred<void>();
let hasThrown = 0;
try {
setTimeout.call(thisArg, () => resolvable.resolve(), 1);
Expand Down Expand Up @@ -286,7 +286,7 @@ unitTest(async function timerMaxCpuBug(): Promise<void> {
unitTest(async function timerBasicMicrotaskOrdering(): Promise<void> {
let s = "";
let count = 0;
const promise = deferred();
const promise = new Deferred<void>();
setTimeout(() => {
Promise.resolve().then(() => {
count++;
Expand All @@ -309,7 +309,7 @@ unitTest(async function timerBasicMicrotaskOrdering(): Promise<void> {

unitTest(async function timerNestedMicrotaskOrdering(): Promise<void> {
let s = "";
const promise = deferred();
const promise = new Deferred<void>();
s += "0";
setTimeout(() => {
s += "4";
Expand Down Expand Up @@ -349,7 +349,7 @@ unitTest(function testQueueMicrotask() {

unitTest(async function timerIgnoresDateOverride(): Promise<void> {
const OriginalDate = Date;
const promise = deferred();
const promise = new Deferred<void>();
let hasThrown = 0;
try {
const overrideCalled: () => number = () => {
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/unit/tls_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
assertEquals,
assertThrows,
assertThrowsAsync,
deferred,
Deferred,
unitTest,
} from "./test_util.ts";
import { BufReader, BufWriter } from "../../../std/io/bufio.ts";
Expand Down Expand Up @@ -121,7 +121,7 @@ unitTest(
unitTest(
{ perms: { read: true, net: true } },
async function dialAndListenTLS(): Promise<void> {
const resolvable = deferred();
const resolvable = new Deferred<void>();
const hostname = "localhost";
const port = 3500;

Expand Down
38 changes: 19 additions & 19 deletions cli/tests/websocket_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
assertThrows,
fail,
} from "../../std/testing/asserts.ts";
import { deferred } from "../../std/async/deferred.ts";
import { Deferred } from "../../std/async/deferred.ts";

Deno.test("invalid scheme", () => {
assertThrows(() => new WebSocket("foo://localhost:4242"));
Expand All @@ -21,7 +21,7 @@ Deno.test("duplicate protocols", () => {
});

Deno.test("invalid server", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:2121");
let err = false;
ws.onerror = (): void => {
Expand All @@ -39,7 +39,7 @@ Deno.test("invalid server", async () => {
});

Deno.test("connect & close", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => {
Expand All @@ -52,7 +52,7 @@ Deno.test("connect & close", async () => {
});

Deno.test("connect & abort", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.close();
let err = false;
Expand All @@ -71,7 +71,7 @@ Deno.test("connect & abort", async () => {
});

Deno.test("connect & close custom valid code", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => ws.close(1000);
Expand All @@ -82,7 +82,7 @@ Deno.test("connect & close custom valid code", async () => {
});

Deno.test("connect & close custom invalid code", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => {
Expand All @@ -96,7 +96,7 @@ Deno.test("connect & close custom invalid code", async () => {
});

Deno.test("connect & close custom valid reason", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => ws.close(1000, "foo");
Expand All @@ -107,7 +107,7 @@ Deno.test("connect & close custom valid reason", async () => {
});

Deno.test("connect & close custom invalid reason", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => {
Expand All @@ -121,7 +121,7 @@ Deno.test("connect & close custom invalid reason", async () => {
});

Deno.test("echo string", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.onerror = (): void => fail();
ws.onopen = (): void => ws.send("foo");
Expand All @@ -136,8 +136,8 @@ Deno.test("echo string", async () => {
});

Deno.test("echo string tls", async () => {
const promise1 = deferred();
const promise2 = deferred();
const promise1 = new Deferred<void>();
const promise2 = new Deferred<void>();
const ws = new WebSocket("wss://localhost:4243");
ws.onerror = (): void => fail();
ws.onopen = (): void => ws.send("foo");
Expand All @@ -154,7 +154,7 @@ Deno.test("echo string tls", async () => {
});

Deno.test("websocket error", async () => {
const promise1 = deferred();
const promise1 = new Deferred<void>();
const ws = new WebSocket("wss://localhost:4242");
ws.onopen = () => fail();
ws.onerror = (err): void => {
Expand All @@ -166,7 +166,7 @@ Deno.test("websocket error", async () => {
});

Deno.test("echo blob with binaryType blob", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
const blob = new Blob(["foo"]);
ws.onerror = (): void => fail();
Expand All @@ -186,7 +186,7 @@ Deno.test("echo blob with binaryType blob", async () => {
});

Deno.test("echo blob with binaryType arraybuffer", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.binaryType = "arraybuffer";
const blob = new Blob(["foo"]);
Expand All @@ -205,7 +205,7 @@ Deno.test("echo blob with binaryType arraybuffer", async () => {
});

Deno.test("echo uint8array with binaryType blob", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
const uint = new Uint8Array([102, 111, 111]);
ws.onerror = (): void => fail();
Expand All @@ -223,7 +223,7 @@ Deno.test("echo uint8array with binaryType blob", async () => {
});

Deno.test("echo uint8array with binaryType arraybuffer", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.binaryType = "arraybuffer";
const uint = new Uint8Array([102, 111, 111]);
Expand All @@ -240,7 +240,7 @@ Deno.test("echo uint8array with binaryType arraybuffer", async () => {
});

Deno.test("echo arraybuffer with binaryType blob", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
const buffer = new ArrayBuffer(3);
ws.onerror = (): void => fail();
Expand All @@ -258,7 +258,7 @@ Deno.test("echo arraybuffer with binaryType blob", async () => {
});

Deno.test("echo arraybuffer with binaryType arraybuffer", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
ws.binaryType = "arraybuffer";
const buffer = new ArrayBuffer(3);
Expand All @@ -275,7 +275,7 @@ Deno.test("echo arraybuffer with binaryType arraybuffer", async () => {
});

Deno.test("Event Handlers order", async () => {
const promise = deferred();
const promise = new Deferred<void>();
const ws = new WebSocket("ws://localhost:4242");
const arr: number[] = [];
ws.onerror = (): void => fail();
Expand Down
Loading