search

nhagen / react-intercom

A component to configure and enable Intercom in your react application
MIT License
237 stars 72 forks source link

Check that window.Intercom exists #17

Closed couds closed 5 years ago

couds commented 8 years ago
nhagen commented 8 years ago

Hey! Thanks for the pull request! Happy to merge it in, but have some questions.

Trying to think of a case where you can unmount a component and constructor isn't called when you mount it again, but drawing blanks. Can you provide and example for me? Also, we still need the initialization to happen in constructor because componentWillReceiveProps does not fire on initial mount.

couds commented 8 years ago

Hi!

About the case where you unmount a component and the constructor its not called when you mount it again, if you have you component instance saved and just hide and show depending of that the constructor will not be called again.

also because you don't need the settings on server render its better to set it in componentDidMount because will be only called on the client (in case of isomorphic code)

And about the initialization, we dont need the constructor because we do it also in the componentDidMount. Also could be better to create a private function to do the init so the code its not duplicated.

And for last, checking in the componentwillUnmount that window.Intercom exists before using it need to be done, In my case the componentWillUnmount was called twice (That part of the code Im not sure why because I didnt do it, but I guess that we have 2 Intercom buttons) and when unmounting the second one fails. Can bo a good Idea to track how many instances of the button exist to know if the call to shutdown intercom need to be done.

I dont know it is relevant but I had this problems and if I had it probably other developers will have it too.

Thanks

nhagen commented 6 years ago

@couds I hope life has treated you well. Not sure why this never got merged in, but I think this should get in, and I'd like to clean up the issues/PRs for this repo.

This PR needs some updating to (1) fix merge conflicts, and (2) address @benjdlambert 's comment, and (3) componentWillReceieveProps is now deprecated--instead, we can just use componentDidUpdate and remove the shouldComponentUpdate check. Since we don't render anything, I think its fine do the no-op the render in order to have a more idiomatic use of other lifecycle methods.

couds commented 6 years ago

Hi @nhagen I totally forgot about this PR XD. I just did a rebase and updated the code a little. can you check this again?

Thanks

krvajal commented 5 years ago

I created a custom React Hook that provides the same functionality as this library https://github.com/krvajal/use-intercom-hook