SplitProvider - Functional and aware of config changes

[CarsonF]

• Changed client to be nullable.
  Now we don't have to fake a default client ({} as SplitIO.Client).
  This will prevent error if client is not defined and we try to call getTreatmentsWithConfig.
• Changed lastUpdate to just be 0 instead of null for default.
  Since it is a unix timestamp check 0 functions fine.
• I'm also setting lastUpdate to "now" when the client is ready, because I'm pretty sure that's the time it has the treatments loaded from server.
• I'm re-creating the client if the config options have changed.
  By giving all the config options as a dependency list to useEffect.
• Converted SplitProvider to a function component.

[samuelcastro]

Looks great, just a few small details.

[CarsonF]

Bleh I did a review instead of single comments 🤦‍♂

[CarsonF]

Ok I'm ready for another review @samuelcastro. I did everything we've talked about so far.

I also added custom handling for debug and impressionListener settings. You can see why in code/commits. If you don't like these or don't think it is necessary, then I can remove.

EDIT: I removed the debug special handling because it was destructuring the config property which would always give it a new identity.

[CarsonF]

I thought of another caveat. Since we are memoize the config object before JSON.stringify if the user tries to mutate the object the changes won't be applied. Mutating the same object isn't idiomatic for react, but it would be nice not to "do nothing". Maybe Object.freeze can be used...

[samuelcastro]

The changes looks great, I like the idea of freezing the object, this way users will know mutating is a bad idea. Or we could use immer and this mutation would never happen, however once again, I'm avoid unnecessary dependencies.

[samuelcastro]

🥇

[samuelcastro]

I'm going to release it as an alpha version after merge and then after make sure everything is good to go we can make a final release. If you want to test it as well I have a Code Sandbox live where we can easily test it: https://codesandbox.io/s/jovial-banzai-3bc81

[CarsonF]

Ok I added the freeze call. I'm now thinking though that we need to tweak again.

<SplitProvider config={ core: { authorizationKey: '' } } />

This will run the recursive deep freeze and JSON.stringify on every render.
I'm not sure that's a good thing.

The only solution might be to tell users to memoize their config or define outside of component.

[samuelcastro]

Yeah I don't like idea either, maybe we should leave it as it is and just let users know mutation will produce unexpected results with the client not being re-created.

[CarsonF]

I liked it more when I didn't think I had to implement recursion myself.

I still think we should tell users the best way to write it so we don't have to do JSON.stringify on every render. And if we are doing that, and assuming users follow it because they care about performance, then the freeze is also not called needlessly. In that case it would be safer to keep the freeze.

[samuelcastro]

Looks good.

[CarsonF]

Ship it! 😁

I'll rebase the other PR next and update docs.