Downshift: Does resetIdCounter() work?

[jaredpalmer]

Currently upgrading an app to React 16 and it seems that downshift is breaking hydration. To confirm, I was able to reproduce on a brand new Razzle app.

See https://github.com/jaredpalmer/downshift-razzle-bug

Thoughts?

[serh11p]

@jaredpalmer, hm... 🤨 We should probably figure out what's going on.
But now I recommend You to:

• Pass id's manually, like:

<Downshift
  id="my-super-select-id"
 {/* ... */}
/>

and

<input {...getInputProps({
  id: 'my-input-id',
  placeholder: 'Favorite fruit ?'
})} />

It overrides auto-generated id's.

• And remove the .resetIdCounter().

_UPDATED: this is not webpack problem (as I thought before)_

[kentcdodds]

The feature is intended for server side rendering and there's actually an existing test for it. Not sure what is going on, but @notruth's workaround will work. You can also pass an ID prop to downshift directly.

But we should probably figure out what's going on 🤔

I won't have time to work on this myself though.

[serh11p]

I found.

This(root) generateId() calls in Init time and idCounter right away rises to 2(default is 1).

On server-side after that we reset the idCounter to 0 every render time. It means that others generateId()'s begin counting from 0.
But on client-side others generateId()'s begin counting from 2, because no reset occurs.

Solution:

• Set default idCounter to 0(instead of 1):

let idCounter = 0

• Migrate this generateId() into constructor() handler, like:

constructor(...args) {
  // ...
  this.id = this.props.id || generateId('downshift')
}
// ...

getItemId(index) {
  return `${this.id}-item-${index}`
}

and never call generateId() in init time.
OR replace resetting logic with code below(hack solution):

/**
 * Resets idCounter to last idCounter value after init. Used for SSR.
 */
let idCounterAfterInit
function resetIdCounter() {
  if (idCounterAfterInit === void 0) {
    idCounterAfterInit = idCounter
  }

  idCounter = idCounterAfterInit
}

[kentcdodds]

That sounds fine to me! Thanks for researching that @notruth!

Two things:

1. Let's definitely add a brief code comment over the initialization of the idCounterAfterInit variable to explain what its purpose is because it seems nonsensical without some context.
2. Instead of void 0 let's just use undefined :)

Thanks!

[serh11p]

@kentcdodds, Thanks for review.
About void 0 - sure.
But I prefer the first solution 'Migrate root generateId() into constructor() handler', it's more semantic: render time = render id. What do you think?
(and dirty hack with idCounterAfterInit variable will not work if the user calls Downshift.resetIdCounter() after React.renderToSomething() 😁)

[kentcdodds]

Oooh! I missed that! That's actually a bug! What it's currently doing is setting an id for _all_ instances of downshift, not each individual instance. Yes, we should definitely fix this. Except, I would remove the defaultProp for the id here (that's the bug), and add id = this.props.id || generateId('downshift') above this line, then do as you suggested with getItemId :+1:

A PR would be appreciated for this. I wonder if there's any way we could add a test for this as well 🤔

[donysukardi]

Yeah. Looking at it, I think the first solution, i.e. moving generateId to constructor makes more sense.

[serh11p]

@kentcdodds, @donysukardi, Thanks for approve.
About test: I'm think it will not be difficult. Just need to check that the idCounter value is equals to 0 after Downshift initialized...

[kentcdodds]

Cool, thanks all! I love working on downshift with you fine folks!

[kentcdodds]

Note: we do currently have tests for SSR: https://github.com/paypal/downshift/blob/master/other/ssr/__tests__/index.js

If you could update those to catch this that would probably be best 👌

[kentcdodds]

Though now that I think about this, I'm not sure that'd be the right place for it. 🤔 Whatever you think is best

[serh11p]

Yeah, probably it's better to do a separate test for this...

[jaredpalmer]

@kentcdodds confirmed workaround is just passing an id to getInputProps() and removing resetIdCounter().

2 Questions:

• From a DX perspective, I was surprised that downshift needed special SSR consideration. What do you think about about calling this out a bit earlier in the documentation?
• How do you feel about making id required in getInputProps() in the interim? (At minimum in TS types and perhaps by explicitly by using invariant() or an assertion)?

[kentcdodds]

What do you think about about calling this out a bit earlier in the documentation?

We do have this mentioned in the FAQ. I suppose we could mention it earlier. Or perhaps we could add an entire section to the docs about server side rendering. That way it would appear in the table of contents. Would you like to do that?

How do you feel about making id required in getInputProps() in the interim? (At minimum in TS types and perhaps by explicitly by using invariant() or an assertion)?

Nope, sorry. 99% of use cases for downshift (more?) don't require SSR. I'm not going to put pain on 99% of users to make the DX of 1% of users _slightly_ better. The id is _not_ required, except for SSR.

Perhaps we could make this happen if you're rendering downshift on the server (no window object or something). I would rather have a warning logged though I think. Thoughts?

[jaredpalmer]

@kentcdodds a warning if window is undefined is a great idea

[kentcdodds]

Cool! Let's do it. Who wants to give it a go?!

[donysukardi]

That wouldn't quite address the actual issue with the id mismatch as @notruth pointed out. I think once we resolve that, providing explicit id should no longer be necessary (as intended in the original design).

@jaredpalmer, to answer your first question, it’s quite a common approach to reset auto generated counter (in this case the id). A similar approach is also used by
https://github.com/reactjs/react-tabs/blob/master/README.md#resetidcounter-void.

[donysukardi]

This should be working as intended with [email protected] now. No explicit id is required 🎉

[kentcdodds]

Thanks!