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: this.state.chans and channel.users are Map type, not an Object type #103

Merged
merged 6 commits into from
May 10, 2023

Conversation

iwalkalone69
Copy link

  • Object.entries(this.state.chans) is not working as it is a Map, it returns an empty array.
  • message.nick in killChannel.users is not working as it is a Map too. Repacing with Map.prototype.has().
  • Replacing delete killChannel.users[message.nick] with Map.prototype.delete().
  • Doing the same with quitChannel.

@iwalkalone69 iwalkalone69 requested a review from a team as a code owner May 2, 2023 07:03
Copy link

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Change looks good, but can you:

Optionally, a test would be good to avoid this. We have a nice example in

test('can join a channel and send a message', async () => {
const { speaker, listener } = server.clients;
await listener.join('#foobar');
const messagePromise = listener.waitForEvent('message');
await speaker.join('#foobar');
await speaker.say('#foobar', 'Hello world!');
const [nick, channel, text] = await messagePromise;
expect(nick).toBe(speaker.nick);
expect(channel).toBe('#foobar');
expect(text).toBe('Hello world!');
});
.

Signed-off-by: iwalkalone <iwalkalone69@gmail.com>
@progval
Copy link

progval commented May 2, 2023

So that's why appservice-irc stopped bridging quits months ago! Thanks @iwalkalone69!

@progval
Copy link

progval commented May 2, 2023

Did you check onNick doesn't need it too?

@iwalkalone69
Copy link
Author

Did you check onNick doesn't need it too?

I didn't check it yet, but probably it will need too. If I can confirm and check it will submit another PR.

@iwalkalone69
Copy link
Author

Did you check onNick doesn't need it too?

I have committed another change that fixes this. It uses Map.prototype.forEach() to iterate channels.

@Half-Shot
Copy link

Incidentally I think this sort of thing really needs a linting rule, but alas there isn't one yet typescript-eslint/typescript-eslint#6807

src/irc.ts Outdated
nickChannel.users.set(message.args[0], chanUser);
this.state.chans.forEach((nickChannel, channame) => {
if (message.nick && nickChannel.users.has(message.nick)) {
nickChannel.users.set(message.args[0], nickChannel.users.get(message.nick));

Choose a reason for hiding this comment

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

Seems like there is a typing fail here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry. It is fixed now.

iwalkalone69 and others added 2 commits May 4, 2023 11:16
Signed-off-by: iwalkalone <iwalkalone69@gmail.com>
Copy link

@Half-Shot Half-Shot 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 bugfix :)

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.

3 participants