Slight refactor of message creation #120
Conversation
Move getCreatedMessageData() from the message stores getters to the ChatMessageUtils and move call to WebAPI's create method into the message store.
|
Yeah, this makes more sense to me, honestly. The chat example was something I put together for a presentation at the ForwardJS conference in the fall of 2014 as an attempt to show a more complicated example than the TodoMVC one, yet still keep it simple. Jing and I wanted to show both waitFor() and web API interactions. Unfortunately, my attempt at increased complexity while staying as simple as possible resulted in an example that was a little bit forced. We wanted to show the API calls coming out of the action creators, as this is the style Jing prefers in most cases. Other folks at FB prefer to make the calls to the web API in the store, as you have done here. I'm pretty neutral about it. There are two different ways to do it, and both are correct. One advantage of doing the XHR call in the action creator instead of the store, as a rule: this makes it impossible to make the mistake of handling the XHR response directly in the store, rather than creating a new action. Then requiring the action creator modules in the store becomes an easily recognizable red flag. But this is really a matter of taste and of team dynamics, I think. I think we should let the current code stand as-is, and not cause too much confusion in the Flux community by changing it at this point. But I'll keep this open and think about it some more. I should probably write up a full explanation of all this in blog post or documentation page, and maybe even develop a new example that shows stores making web API calls. |
|
Sorry, my intent wasn't to change the location the API calls were made from. All of your points above are valid and I agree that it makes sense to leave the API call in the action creator for this example. My intent was to move the Would it make sense to pass the current thread id in the payload with the message text, or do you think adding the extra dependency to the |
|
ok that's cool, we can do that. If you revise this in that way, I'll merge it in. |
|
Sorry I forgot about this. I will try and remember to do it after work this evening... |
|
@fisherwebdev sorry for the series of delays. Is this an acceptable compromise of not making an API call from the store, but factoring the utils method out of the store? |
| @@ -21,6 +21,18 @@ module.exports = { | |||
| text: rawMessage.text, | |||
| isRead: rawMessage.threadID === currentThreadID | |||
| }; | |||
| }, | |||
|
|
|||
| getCreatedMessageData: function(text, currentThreadID) { | |||
There was a problem hiding this comment.
I might have named this createMessageData, as it's more generative, not a really a query. Probably should have been named that originally, but I was thinking in terms of store getters.
There was a problem hiding this comment.
Yeah, that makes more sense. I just copied the name from the original method in the store.
Slight refactor of message creation
Removing unused store. Change related to #120
Since the
getCreatedMessageDatamethod of theMessageStoreis just a message specific utility, it makes more sense to put it inChatMessageUtils. The debatable part here is that I then moved the call to the APIs create method to the store. The primary motivation for that was the availability ofcurrentThreadID, however, this may also be more maintainable because it keeps the optimistic store update close to the server/API create call.