Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

use stream.opened instead of stream.connection #66

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

Tadwork
Copy link
Contributor

@Tadwork Tadwork commented Nov 5, 2023

Deno changed the WebSocketStream API to rename the connection property to opened. This proposed change would add a quick check to see which version of the API is in use and use the appropriate property. This should fix render-oss/homebrew-render#12

  • Tested using Deno 1.37.2 by running deno task run services tail --id XXXXXXX to ensure that tails works as expected with the check

@Tadwork Tadwork force-pushed the fix-websocket-error-in-tail branch from ae97f64 to 89abf36 Compare November 5, 2023 04:49
@Tadwork Tadwork force-pushed the fix-websocket-error-in-tail branch from 89abf36 to 9aedc24 Compare November 5, 2023 05:00
@stefanluptak
Copy link

What's the ETA of this MR review? Not being able to install the CLI for weeks is quite an issue...

@tfreeborough
Copy link

+1 Please can we get this reviewed and merged?

Copy link
Contributor

@shawn-render shawn-render left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@@ -51,5 +51,5 @@ Deno.test('findUp', async (t) => {
assertEquals(ret, null);
});

Deno.remove(root, { recursive: true });
await Deno.remove(root, { recursive: true });
Copy link
Contributor Author

@Tadwork Tadwork Dec 5, 2023

Choose a reason for hiding this comment

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

@shawn-render added this to fix the flaky test runs because otherwise the tests would fail in many cases with

findUp => ./util/find-up.test.ts:10:6
error: Leaking async ops:
  - 1 async operation to op_fs_remove_async was started in this test, but never completed. The operation was started here:
    at handleOpCallTracing (ext:core/01_core.js:610:42)
    at Object.opAsync (ext:core/01_core.js:603:15)
    at Object.remove (ext:deno_fs/30_fs.js:199:14)
    at file:///Users/tzvifriedman/dev/render-cli/util/find-up.test.ts:54:8
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async innerWrapped (ext:cli/40_testing.js:544:5)
    at async asyncOpSanitizer (ext:cli/40_testing.js:193:27)
    at async resourceSanitizer (ext:cli/40_testing.js:426:25)
    at async exitSanitizer (ext:cli/40_testing.js:475:27)
    at async outerWrapped (ext:cli/40_testing.js:489:14)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I was just looking into that too but you beat me to it 😄

@Tadwork
Copy link
Contributor Author

Tadwork commented Dec 5, 2023

@shawn-render I removed the check for stream.connection since it was failing the compile stage. The previous check was unnecessary since the compile stage will bundle the appropriate version of Deno anyway

@@ -59,7 +59,7 @@ export const servicesTailCommand =
let writer: WritableStreamDefaultWriter<string | Uint8Array>;

try {
conn = await stream.connection;
conn = await stream.opened;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawn-render I removed the check for stream.connection since it was failing the compile stage. The previous check was unnecessary since the compile stage will bundle the appropriate version of Deno anyway

@shawn-render shawn-render merged commit e40cb38 into render-oss:main Dec 5, 2023
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue installing from brew
4 participants