[Slider] React.StrictMode fixes

[jjenzz]

---

NOTE: PR will be easier to review with hidden whitespace changes

---

Fixes #615
Fixes #773

Replaces our old collection logic with our new Collection. This fixes the React.StrictMode issues that have been reported lately, but means the thumbs will no longer be in the correct position on initial render for SSR as our new Collection component doesn't handle this case.

More information here. Thanks to @andy-hook for narrowing down collection code as the issue.

It seems this is a less jarring experience for now so I'm thinking we can look into improving SSR experience across our components separately.

Tested in:

• NextJS React.StrictMode
• Create React App React.StrictMode
• Snowpack React.StrictMode

CleanShot.2021-07-22.at.13.18.18.mp4

[benoitgrelard]

Nice work on this! 👍

[benoitgrelard]

If we're checking for the initial render case here, is the index !== -1 just a safety to not render the whole thing if for some reason that thumb wasn't tracked? I can't think of any reason why this check would be necessary right now.

[jjenzz]

It's necessary for this case:

<Slider.Root value={[20]}>
	...
	<Slider.Thumb />
	<Slider.Thumb />
</Slider.Root>

Notice how there are two thumbs but only one value? The second thumb won't render. We handled this case before with the value !== undefined check but perhaps it's okay to let them render a thumb that does nothing and expect them to remove the redundant JSX instead?

[benoitgrelard]

Yeah that’s probably fine too seeing as it’s just a user error that they would figure out pretty easily.

[jjenzz]

I can create a PR for that then 👍

[benoitgrelard]

Might have been ok to leave it as is rather than doing more work specifically to hide it?
Are you thinking that we'll deal with this anyway differently at some point?

[jjenzz]

You think it's okay for them to render in the wrong position and snap into a different position during hydration?

The plan is to fix the SSR issue eventually so that it renders in the correct position server-side.

[benoitgrelard]

It’s not great but I wonder how visible it is in a real use case. But yeah regardless SSR is something we need to look at some point. We might end up with some controlled overrides (like providing the index yourself) when you want to make sure it’s ok on first server render.

[jjenzz]

We might end up with some controlled overrides (like providing the index yourself)

I was thinking this too since we already have the index prop on the Impl part. We just don't expose it via the SliderThumb part atm.

[benoitgrelard]

I think these kind of solutions would definitely be the easiest. Simple trade offs.