Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix r10 tabs test #2702

Merged
merged 3 commits into from
Feb 4, 2020
Merged

Fix r10 tabs test #2702

merged 3 commits into from
Feb 4, 2020

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Feb 3, 2020

WHY are these changes introduced?

Fixes #2694

WHAT is this pull request doing?

Removing the setTimeout mock

How to 🎩

  1. git reset HEAD~2 --hard
  2. dev up # or yarn install
  3. yarn test TabMeasurer

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2020

🟢 No significant changes to src/**/*.tsx were detected.

@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review February 3, 2020 15:58
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

This fixes the tests but I'm worried that this is just fixing a symptom rather than a root cause. I'm curious about why we need that setTimeout call in the first place - and why only in development mode?

Some folks appear to be having problems on the initial render - see the comment on the original issue and https://discourse.shopify.io/t/reloading-ssr-application-causes-polaris-tabs-to-load-differently/6389.

Perhaps the measurement handling should be done with a useLayoutEffect rather than useEffect? (which will need to be wrapped in some custom hook because it doesn't play nice with SSR)

@AndrewMusgrave
Copy link
Member Author

This fixes the tests but I'm worried that this is just fixing a symptom rather than a root cause. I'm curious about why we need that setTimeout call in the first place - and why only in development mode?

I'm not sure. IIRC at one point in time, styles loading was delayed. Just a shot in the dark but they may have been needed for measuring.

Some folks appear to be having problems on the initial render - see the comment on the original issue and https://discourse.shopify.io/t/reloading-ssr-application-causes-polaris-tabs-to-load-differently/6389.

@dleroux Mentioned we've had this issue for a while, so I don't think it's related to the react bump.

Perhaps the measurement handling should be done with a useLayoutEffect rather than useEffect? (which will need to be wrapped in some custom hook because it doesn't play nice with SSR)

If we do need to delay measurements, in production that would be the way to go 👍 However I don't think that'll fix the test. React uses setTimeout in their scheduler and I'm betting our mock breaks it 😄

@AndrewMusgrave
Copy link
Member Author

For reference plus created an issue #2706

@AndrewMusgrave AndrewMusgrave merged commit 6a10c02 into master Feb 4, 2020
@AndrewMusgrave AndrewMusgrave deleted the fix-r10-tabs-test branch February 4, 2020 22:55
@tmlayton tmlayton temporarily deployed to production March 7, 2020 05:24 Inactive
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.

TabMeasurer tests fail in React >=16.10
3 participants