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

Type fixes needed to make matrix-appservice-irc build #196

Merged
merged 7 commits into from
Aug 10, 2020

Conversation

Half-Shot
Copy link
Contributor

This is a collection of changes needed to make our types work with the IRC bridge. We should either merge this into the parent PR (and subsequently into that one), or find a way to include these changes.

// eslint-disable-next-line camelcase
room_id: string;
// eslint-disable-next-line camelcase
state_key: string;
type: string;
// eslint-disable-next-line camelcase
event_id: string;
content: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is content guaranteed to be an Object or claimed as here unknown? That would require some more type checks in my PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably call it a Record<string,unknown>

Copy link
Contributor

Choose a reason for hiding this comment

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

But if that's not guaranteed by the client lib, we'll get errors like the one #155 fixes, where a presumed object may be a string or a number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content should really never be anything other than a object, if it is, we should reject it before insertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Ok

@jaller94 jaller94 changed the base branch from j94/typescript-intent+8.0 to j94/typescript-intent August 10, 2020 18:06
@jaller94 jaller94 merged commit 84a322e into j94/typescript-intent Aug 10, 2020
@jaller94 jaller94 deleted the hs/typing-fixes branch August 10, 2020 18:06
// Reject - unexpected content type
return;
}
if (!event.type || !event.state_key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for learning something: This broke the tests. It's not testing if these keys are missing (as claimed by the line below), but if they are truethy. The tests use empty strings, which aren't truethy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in deb2f38

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