upmostly / modali

A delightful modal dialog component for React, built from the ground up to support React Hooks.
212 stars 23 forks source link

Modali uses shortid & nanoid at the same time #26

Open jaxxreal opened 4 years ago

jaxxreal commented 4 years ago

Hello!

That lib looks really promising, so thank you!

I found an issue exploring modali with bundlephobia.

From bundlephobia stats, it turns that modali uses shortid & nanoid in the same time. A purpose of these libs is the same, but shortid is 33% of the modali bundle, which is huge.

Снимок экрана 2019-07-24 в 11 06 12
mikaello commented 4 years ago

shortid uses nanoid, that is why both show up in bundlephobia. shortid is considered a dead project (the current maintainer states this multiple times in the issues and PRs), and nanoid is considered a better alternative.

In addition, the way the random generator is used in this project is a bit troublesome, as it generates new IDs for every render of the footer.

nanoid has a comment about this:

Do not use a nanoid for key prop. In React key should be consistence between renders. This is bad code:

<Item key={nanoid()} /> /* DON’T DO IT */

This is good code. this.id will be generated only once:

  id = nanoid()
  render () {
    return <Item key={this.id}>;
  }
}
jaxxreal commented 4 years ago

I guess nanoid doc is trying to protect React newcomers. Taking into account, that you change key each render intentionally, it doesn't look like a big deal to throw out shortid and use nanoid only. I can author a PR if needed.