Conversation
eldh
left a comment
eldh
left a comment
There was a problem hiding this comment.
Just one comments, otherwise it looks good I think. Maybe @JoelBesada also wants to have a look.
| const tide = { | ||
| keyPaths: this.getKeyPaths(), | ||
| ...this.tide.getComponentProps(), | ||
| } |
There was a problem hiding this comment.
I think it makes sense to keep this in the constructor so it's not run each render.
There was a problem hiding this comment.
Keep in mind that we need to update it whenever the props change.
There was a problem hiding this comment.
We currently don't, right? They're set as this._componentTide and assumed to be static.
There was a problem hiding this comment.
It looks like this section could do with a little bit of a reorganizing as well?
As far as I can tell, the tide prop contains stale keyPaths, which are not used anywhere. So really, the only thing that is interesting in the tide prop, is that actions are passed in.
I also looked into doing a better job of caching getKeyPaths, my guess is that is the most expensive operation per render cycle. I decided not to for readability reasons, wdyt?
There was a problem hiding this comment.
I think we want to prioritise performance over readability here. Might be worth doing a benchmark to check what the effects might be.
There was a problem hiding this comment.
Sure, check out the benchmarking here: https://gist.github.com/lwakefield/8e8a702dd9b8df7679174e468bad8706
The refactor is a little slower (~3%). Let me see what I can do, but I think we can do better than the existing solution!
There was a problem hiding this comment.
Made some tweaks, the variance is pretty wild when profiling, as we are looking at a very specific part. But we are now seeing ~2-5% perf improvement.
|
@lwakefield is this going live? |
4 participants
This PR is intended to improve the readability of
TideComponent. I tried to make it "un-opinionated", though I can't vouch for that!Branched from
clean-up-warnings, so if this gets merged, make sure to rebase!