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.

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 code example #393

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Borewit
Copy link

@Borewit Borewit commented Feb 2, 2025

While posting answer on StackOverflow using mocket-socket, I found a few issues in the sample code.

Changes:

README.md Outdated
t.is(app.messages[0], 'test message from mock server', 'we have stubbed our websocket backend');
mockServer.stop(t.done);
}, 100);
await new Promise(resolve => setTimeout(resolve, 100));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to avoid using setTimeout at all. I think it's worth adding some helpers.

const waitFor = (connection, event) => new Promise(resolve => {
    const handler = () => {
        resolve();
        connection.removeEventListener(event, handler);
    }
    connection.addEventListener(event, handler);
});


const app = new ChatApp(fakeURL);
await waitFor(app.connection, 'open');
...
await waitFor(app.connection, 'message');

Copy link
Author

Choose a reason for hiding this comment

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

The are quite some issues with suggestion provided, so I used an alternative approach.

@Atrue
Copy link
Collaborator

Atrue commented Feb 7, 2025

@Borewit Sorry for the long delay. Can you please fix the same for the socket.io part of the README?

Changes:
- Fixes trying to emit a connection when the WebSocket is not yet connected.
- Updated AVA test, resolving thoov#388
- Convert incoming messages to `AsyncQueue`
@Borewit Borewit force-pushed the readme-fix-code-example branch from 5eff5f5 to 78adba6 Compare February 7, 2025 14:42
@Borewit
Copy link
Author

Borewit commented Feb 7, 2025

@Borewit Sorry for the long delay. Can you please fix the same for the socket.io part of the README?

Sorry, I will keep with PR with that example. To many things on my plate.

@@ -37,23 +37,35 @@ import { WebSocket, Server } from 'mock-socket';
```js
import test from 'ava';
import { Server } from 'mock-socket';
import {AsyncQueue} from '@borewit/async-queue';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to keep the readme simple and avoid using 3rd party libs

Copy link
Author

Choose a reason for hiding this comment

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

I fully understand that. I tried to embed an inline solution, yet that started to a bit lengthy and distracting, hence I moved this portion to borewit/async-queue.

I fixed the code example in 3 different area's. I leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants