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

Modali uses shortid & nanoid at the same time #26

Open
jaxxreal opened this issue Jul 24, 2019 · 2 comments
Open

Modali uses shortid & nanoid at the same time #26

jaxxreal opened this issue Jul 24, 2019 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@jaxxreal
Copy link

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

@jcrowson jcrowson self-assigned this Jul 24, 2019
@jcrowson jcrowson added the enhancement New feature or request label Jul 24, 2019
@mikaello
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants