Skip to content

fix(react): correctly stabilize transformItems function#110

Merged
marialungu merged 6 commits intonextfrom
fix/transform-items-bug
Nov 14, 2022
Merged

fix(react): correctly stabilize transformItems function#110
marialungu merged 6 commits intonextfrom
fix/transform-items-bug

Conversation

@marialungu
Copy link
Copy Markdown
Collaborator

Correctly stabilize transformItems function from the hooks, as reported in this issue

Comment on lines +28 to +29
const transformItemsRef = useRef(userTransformItems);
const transformItems = transformItemsRef.current;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some tests by updating the transformItems prop? You'll realize this doesn't work.

You need to reassign transformItemsRef.current to the transformItems prop in an effect, and then use transformItemsRef.current in the existing effect.

});

act(() => {
rerender();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests that the weird array behavior is gone, but it doesn't test that the feature actually works.

Here, we want to rerender with another transformItems reference, and to make sure that the items have been transformed with this latest function reference.

Comment thread packages/recommend-react/src/useFrequentlyBoughtTogether.ts Outdated
Comment thread packages/recommend-react/src/useFrequentlyBoughtTogether.ts
Comment thread packages/recommend-react/src/__tests__/useFrequentlyBoughtTogether.test.ts Outdated
@francoischalifour
Copy link
Copy Markdown
Contributor

@Haroenv @sarahdayan @dhayab Are you happy with this function stability solution?

It automatically "caches" the transformItems function in a ref so that users can pass unstable function references.

However, when the transformItems function reference changes, it doesn't re-trigger a render. It's only on the next non-function prop change that the effect re-executes.

@Haroenv
Copy link
Copy Markdown
Contributor

Haroenv commented Nov 14, 2022

If you're purposely changing the transformItems function you would expect it to immediately be evaluated, however if you simply pass an unstable function you wouldn't expect that to have a side effect. As those behaviours are incompatible, the most correct is indeed probably what you're suggesting here; i.e. stabilising the function, but not causing any side effects through any changes that would happen

@marialungu marialungu merged commit 3384224 into next Nov 14, 2022
@marialungu marialungu deleted the fix/transform-items-bug branch November 14, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants