ts-odd icon indicating copy to clipboard operation
ts-odd copied to clipboard

Change redirectToLobby to accept a single object parameter

Open bgins opened this issue 4 years ago • 2 comments

Summary

Problem

redirectToLobby requires default arguments to fill positional arguments in some cases.

Impact

It may be inconvenient or feel like boilerplate for users to fill in defaults for these arguments.

Solution

Update redirectToLobby to accept an object with permissions, redirectTo, and extraLobbyParams.

Detail

Is your feature request related to a problem? Please describe.

The problem will only surface when the extraLobbyParams in #273 are added. When these changes are added, users will always need to pass a value for redirectTo in order to use the extraLobbyParams.

Describe the solution you'd like

It would be better if we passed a single object to redirectToLobby, for example:

redirectToLobby({ permissions: state.permissions, extraLobbyParams: { theme: <CID> } }) 

would allow users to skip redirectTo.

Describe alternatives you've considered

It might be possible to use arrays or some other data structure, but objects seem idiomatic for this.

Additional context

This will be a breaking change, but it isn't super impactful. We should wait to bring it in with other breaking changes to minimize disruption to codebases that are currently using webnative.

bgins avatar Aug 06 '21 21:08 bgins

Throwing in another idea:

redirectToLobby(state.permissions, { extraLobbyParams: ..., redirectTo: ...})

I'd prefer this, because it seems to be a common pattern. There's one "main", required argument (in this case the permissions), and then an optional second parameter "options", which is then a record where each field is optional.

Small win: Would still be a breaking change, because the second parameter redirectTo would become a record, but it's not that much of a breaking change, because everyone using redirectToLobby with only the permissions argument wouldn't get broken.

matheus23 avatar Aug 09 '21 12:08 matheus23

Throwing in another idea:

redirectToLobby(state.permissions, { extraLobbyParams: ..., redirectTo: ...})

I'd prefer this, because it seems to be a common pattern. There's one "main", required argument (in this case the permissions), and then an optional second parameter "options", which is then a record where each field is optional.

Small win: Would still be a breaking change, because the second parameter redirectTo would become a record, but it's not that much of a breaking change, because everyone using redirectToLobby with only the permissions argument wouldn't get broken.

Agreed. That totally makes sense and makes it explicit that permissions are required. And I think the change would break a much smaller percentage of apps.

bgins avatar Aug 09 '21 15:08 bgins