Refactor utils into stand-alone functions

[scarmuega]

What was done:

• moved isSameUser and isSameDay functions into stand-alone utils.js file
• refactored all components that needed those functions so that they consume the new ones
• removed all code that was used to propagate the original functions through props
• removed requirements from propTypes and defaultProps

[dgdavid]

You can take the opportunity and also refactor this using the isSame moment's function.

Something like

return moment(diffMessage.createdAt).isSame(currentMessage.createdAt)

[dgdavid]

Here one if statement could be avoided, although the line length would be greater.

if (diffMessage.user && currentMessage.user && diffMessage.user._id === currentMessage.user._id) {
  return true;
}

[kfiroo]

@scarmuega Looks solid, @FaridSafi what do you think?

[scarmuega]

@dgdavid thanks for the feedback, I agree with the proposed optimizations, but my approach was to avoid any extra modification to original source code, to minimize the impact of the PR.

With that being said, if @kfiroo and @FaridSafi agree, I can apply some adjustments within the utils.js file before closing the PR.

[dgdavid]

I supposed that, but as you said if they are agree you could to apply more changes that improve both, legibility and performance of code.

BTW, nice job!

[kfiroo]

@scarmuega Nice job indeed!
I say let's do the changes @dgdavid proposed, it's not a big change and it improves readability and performance.
Moreover, it's in a new file so it doesn't change the original code any more :)

[scarmuega]

@dgdavid @kfiroo please check the new commit

[dgdavid]

According with isSame documentation, you can limit the granularity directly:

return currentCreatedAt.isSame(diffCreatedAt, 'day')

[scarmuega]

didn't know about the granularity param of the isSame function. I'll test it and push it.

[dgdavid]

It looks good.

But I only to want share a thought about this: we can even to do something like

return diffMessage.user && currentMessage.user && diffMessage.user._id === currentMessage.user._id

The risk is that it could return an undefined value - valid for this case - and, in my opinion, is less clear.

[kfiroo]

You can always do:

return !!(diffMessage.user && currentMessage.user && diffMessage.user._id === currentMessage.user._id)

[kfiroo]

@scarmuega I think I found another issue with this PR, the isSameUser and isSameDay methods used to be passed as props to all components and all custom renderXXX functions.
For example, when overriding renderTime you should be able to use

renderTime(props) {
    if (props.isSameUser(props.currentMessage, props.nextMessage)) {
        // Do something
    }
} 

Any idea on how to go about this?

[scarmuega]

@kfiroo my idea is that implementors may use these functions by importing them directly, like this:

import { GiftedChat, Time, utils } from 'react-native-gifted-chat';

Then, they can use them in any renderXXX like this:

renderTime(props) {
    if (utils.isSameUser(props.currentMessage, props.nextMessage)) {
        // Do something
    }
} 

[dgdavid]

@scarmuega, @kfiroo perhaps this information deserves either a section in the README or an entry in the Wiki if finally the changes are accepted. Or even both, a warning in the README file and a more detailed example(s) in the Wiki.

[kfiroo]

@scarmuega @dgdavid Hey guys, The only thing left open here is the backward compatibility issue.
Users who override some renderXXX function, like renderTime for example, might have in their code something like

renderTime(props) {
    if (props.isSameUser(props.currentMessage, props.nextMessage)) {
        return (
            <Time
                {...props}
                textStyle={someCustomTextStyles}
            />
        );
    }
    return (
        // SOMETHING ELSE...
    );
},

So I think we should pass these methods as props to these override methods at least.
What do you think?

[dgdavid]

@kfiroo It is a difficult decision, and honestly I don't know what I'd do. But, Did you think about a not backward-compatible update?

I believe that RNGC deserves a major update with all PR that will be accepted and maybe is moment to break the backward compatibility. Why not take the step from 0.0.10 to 1.0.0?. There are a lot of changes pending for be accepted.

Just is an idea. It not answer your question, I know, but just now I see no point to keep these props bloating the component.

[kfiroo]

@dgdavid I will work on a new version with breaking changes, it's on my todo list :)

I still feel like giving people a heads up, like maybe a warning about deprecation before completely breaking something is fair.

[dgdavid]

@kfiroo

I will work on a new version with breaking changes

Nice! I very happy to read that.

I was also looking for some free time to review all issues and PR and make a summary for you and @FaridSafi. But free time also flies 😄

[kfiroo]

@dgdavid I know what you mean! No worries we will get this going soon! :)

[scarmuega]

@kfiroo @dgdavid sounds good. Just to double check:

• I'll restore the missing props to maintain backward compatibility
• Internally, they will work as a wrapper of the new functions in the utils file
• I'll add a deprecation warning (console.warn??) inside the wrapper that travels inside props
• In next major release, these props would be removed completely

Let me know if this is what you had in mind and I'll push the new commit.

[benjaminb10]

@scarmuega finally did you add a utils file?

[scarmuega]

@benjaminb10 yes, if you are pulling from this PR, you should be able to use import { GiftedChat, utils } from 'react-native-gifted-chat' and then you can call utils.isSameDay or utils.isSameUser

[scarmuega]

@dgdavid @kfiroo would you take a look at the last commit? I added backward compatibility and a deprecation warning. Thanks.

[benjaminb10]

@scarmuega I prefere wait for the merge.

[kfiroo]

@scarmuega @dgdavid Good job guys :)