Mattermost-server: Migrate to Flow file src/actions/websocket.js in mattermost-redux

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Mattermost is migrating mattermost-redux library to Flow and we're looking for contributors to help with that effort. This Help Wanted issue is to migrate src/actions/websocket.js to Flow.

Before you start, make sure you read this short Documentation

You can also see an introductory video here: https://www.youtube.com/watch?v=kuHypTgU1ZY

If you need other examples you can see the PRs mattermost/mattermost-redux#488 and mattermost/mattermost-redux#489

[chrux]

I'm taking this!

[hanzei]

Hey @chrux,

Do you have any questions about this ticket?

[hanzei]

Making this available to the public because there wasn't an update from @chrux for some time.

[chrux]

Sorry, I haven't been able to go through the PR and apply the changes

[hanzei]

Do you still want to work on this @chrux?

[chrux]

I don't have time, so let's leave as Up for grabs and if I am able to dedicate some time I will come back, thanks

[tclain]

I can take it if you want guys. Any specifics I need to know about this ?

[hanzei]

Thanks @tclain for picking this one up! :+1:

You might want to take a look at the two licked PR's in the description. They are a good example and guideline.

[tclain]

cool ! thanks this one pretty hard, I'll see what I can do.

[hanzei]

Hey @tclain,

How is the work on this ticket going? Do you have any questions?

[hanzei]

Making this one available for the public again due to inactivity.

[mzaks]

I'm taking this!

[hanzei]

Thanks @mzaks. Let us know, if you have any questions.

[mzaks]

@hanzei I actually have one :)
I did the conversion already and just a couple of minutes ago I had a look at https://github.com/mattermost/mattermost-redux/pull/849
Where there is a similar issues which I stumbled upon: ActionFunc being asynchronous.
I already migrated all the function to be async, but than I read the discussion and it seems to me that people are rather in favour of changing the ActionFunc type to be both.
However the discussion was happening about 10 days ago and I don't see any "resolution"

[mzaks]

I did some testing. If you define the Thunk type as following:

type Thunk = (DispatchFunc, GetStateFunc) => (Promise<ActionResult> | void);

Than there is no need for async and no need for {data: true}.
Which were both discussed as negative changes in the other PR.

[jasonblais]

@jespino or @hmhealey wondering if you'd be able to help on the above?

[jespino]

@mzaks sorry the delay in my reply, I think allowing sync/async functions would be a good thing, but I would keep the ActionResult type because enforces consistency in the returns, and I prefer that.

Your definition, I think is not exactly what is expected. If I'm right, that allows you to return async function that return an ActionResult or a sync function that returns void. I would suggest something like:

type Thunk = (DispatchFunc, GetStateFunc) => (Promise<ActionResult> | ActionResult);

[mzaks]

@jespino No worries about the delay.

Regarding the Thunk type, I specified the return type as void based on the patterns I saw in the websocket.js file. In websocket.js there is a certain "class" of functions which "dispatch schedulers" and by this I mean they call dispatch based on some logic without waiting for the response. There are many such functions starting from here:
https://github.com/mattermost/mattermost-redux/blob/master/src/actions/websocket.js#L197

I can introduce a return {data: true} statement for all of them or wrap the calls to those handle... functions in a function which calls the handlers and returns {data: true}, but I am personally not sure if it is helpful. However the type Thunk is defined globally for all actions and maybe it is just the websocket.js which is out of the ordinary. @jespino Please let me know what you think.

[jespino]

I think if the function is just "fire and forget" you still can return the {data: true} as a result. My concern is about being too permissive in a type to allow to express this case and then end up returning wrongly in other places. I prefer a bit extra data here. for me, if the action is a "fire and forget" as soon as you fire the action, you succeed, so... {data: true} looks sensible to me.

[mzaks]

Than it is settled 😀. I will add {data: true}.

[jespino]

Thanks! :tada:

[hanzei]

@mzaks Do you have any outstanding questions?

[mzaks]

@mzaks Do you have any outstanding questions?

No, all good. As far as I remember, the PR addressing this issue already went through in July.

[mzaks]

@mzaks Do you have any outstanding questions?

No, all good. As far as I remember, the PR addressing this issue already went through in July.

Yes here it is:
https://github.com/mattermost/mattermost-redux/pull/875

[jasonblais]

Thanks @mzaks, closing this issue. Thank you for the contribution!