Closed BRKalow closed 5 years ago
Thank you for this, awesome PR. Thank you especially for the additional test case.
I’m almost inclined not to bump semver major since folks aren’t supposed to be interacting with the context shape directly, but instead rely on the interface of the props.tracking
object shape. Will think about that one for release -- curious to hear your thoughts on this as well.
@tizmagik Thanks for the quick review! I've addressed all your comments.
Regarding whether or not to bump a major version, my first thought was also the same as yours. Since the context shape is intended to only be used internally and it is not a public, documented API it would be acceptable to leave this change as a minor version bump. 👍
...maybe the context type needs to be exported as __INTERNAL_DO_NOT_USE_ReactTrackingContext
😄
Actually I'm wondering if we should be exporting the context type at all? I'm not sure there's a use case for that since the hook is importable as { useTracking }
... 🤔
@tizmagik Thats a good point. With the new hook, it should provide the methods needed to make additional custom hooks. It would reduce the public API surface and perhaps give more flexibility going forward for updating the internals.
Thanks, I'll merge this and then submit a separate PR to remove the context type export since I don't think we want folks to be using that interface at all. Or, if there is a use case, we can follow-up with a separate issue/PR in the future.
This was released in 7.1.0 https://github.com/nytimes/react-tracking/releases/tag/v7.1.0
The React 16 context has slightly different behavior when dealing with updates in a subtree. With the legacy context, a
shouldComponentUpdate
returning false in a component tree would cause the context consumers in its subtree to not receive context updates. From the React docs:With the new context introduced in React 16, the context consumers will always update regardless of a parent's optimizations:
What we encountered within our application was that after the upgrade we were seeing significantly more re-renders of our component trees than before, causing some performance issues.
We were able to fix the re-render regressions by rewriting how the context is structured and how data is merged in a tree of components decorated with
track
.Instead of eagerly computing the tracking context data, we are providing a callback on the context that will merge the data bottom-up. We also changed the dispatch function to use a similar mechanism. This has the benefit of the context value having stable referential equality, so there are no extra renders caused by the context provider updating with the eagerly computed tracking data.
We forked
react-tracking
internally and made these updates, which we have been using for a few weeks. These changes brought our performance metrics back to where they were before thereact-tracking@6
upgrade. We also added a test case in thee2e
test suite which represents the expected behavior (the test case fails on the current master branch).This technically changes the context shape as we removed
context.tracking.data
and replaced it withcontext.tracking.getTrackingData()
, so it will require a major version bump.Simple reproduction case: 6.0.0 behavior: https://codesandbox.io/s/frosty-bouman-7hfmr 5.7.0 behavior: https://codesandbox.io/s/mmq0zn57jp
Thanks!
Co-authored with: @jacekradko