RFC: Use io-ts to validate user input.

[BenPope]

RFC: This implements checks for RemoveRoomRoleForUserOptions only.

• How much existing code will break; should the checks be for values only?
• Is there a better way to allowJs without removed declaration: true
• Are the dependencies ok?
• is Update io-ts, fp-ts, typescript #67 OK?
• Is the new semver ok?

What?

Define the requirements in terms of io-ts types and check them at runtime.
Add tests for Room Roles

Why?

When the typescript is converted to javascript, all type checks are lost.

---

• CHANGELOG updated if relevant?

[minaorangina]

This is fine in theory. It's hard to know if this doesn't break anything, in particular after the removal of Babel. It seems like it's unused, but I'm not wholly confident that the tests would pick this up 😅

[minaorangina]

May as well bump to 2.8.3, which includes some bug fixes

[minaorangina]

Though it wouldn't make a difference here, triple equals is always preferred (i.e. !==)

[minaorangina]

The use of the same name for two different things is confusing at a glance (though I know you mentioned the docs gave an example like this)

[callum-oakley]

I like it. Hadn't seen io-ts before. It is a whole bunch of extra machinery though... and I'd be happier if we were using it everywhere, but maybe using it in this one method is a good test bed for whether we want to put the work in to do it for the whole API?

When you say should the check be for values only... do you mean that some people might be feeding numbers to removeRoomRoleForUser, and it wouldn't have been an issue before, but now it will be? If so I don't feel strongly about it. The public interface has always been documented as taking strings, so it's not strictly a breaking change to start rejecting non-strings.

I think the real issue here is with the API being too "clever" and interpreting an empty string room ID as indicating a global role. Maybe we should change that in V7 instead and then all the SDKs would get this fix for free. I'd be surprised if this issue didn't also feature in all our other SDKs.

[callum-oakley]

can pull this in from https://github.com/gcanti/io-ts-types#usage

[BenPope]

It seems to be more restrictive; it won't take an normal string.
error TS2322: Type 'string' is not assignable to type 'Branded<string, NonEmptyStringBrand>'. Type 'string' is not assignable to type 'Brand<NonEmptyStringBrand>'.

[callum-oakley]

ah I see. never mind then

[BenPope]

I like it. Hadn't seen io-ts before. It is a whole bunch of extra machinery though... and I'd be happier if we were using it everywhere, but maybe using it in this one method is a good test bed for whether we want to put the work in to do it for the whole API?

The intention is to use it everywhere.

When you say should the check be for values only... do you mean that some people might be feeding numbers to removeRoomRoleForUser, and it wouldn't have been an issue before, but now it will be?

More likely it would be a parameter on another method, perhaps messageId.

If so I don't feel strongly about it. The public interface has always been documented as taking strings, so it's not strictly a breaking change to start rejecting non-strings.

Fair enough.

I think the real issue here is with the API being too "clever" and interpreting an empty string room ID as indicating a global role. Maybe we should change that in V7 instead and then all the SDKs would get this fix for free. I'd be surprised if this issue didn't also feature in all our other SDKs.

That makes sense to me.