ai / nanoid

A tiny (124 bytes), secure, URL-friendly, unique string ID generator for JavaScript
https://zelark.github.io/nano-id-cc/
MIT License
24.34k stars 789 forks source link

Attempt to make the customAlphabet API more similar to nanoid #348

Closed stefansundin closed 2 years ago

stefansundin commented 2 years ago

I want to use a custom alphabet, but I find that going from nanoid(size) to nanoid = customAlphabet(alphabet, size) is a bit cumbersome.

Wouldn't it be better if the function returned by customAlphabet behaved exactly like nanoid? That way I could just change how nanoid is defined and the rest of my code would continue to work, since there are nanoid calls that generate IDs of different lengths scattered throughout the code.

So instead of:

https://github.com/ai/nanoid/blob/7528a7362ba2515d200dc60c3f00f162e7ba563e/non-secure/index.js#L10-L11

It would use:

let customAlphabet = (alphabet) => {
  return (size = 21) => {

Just like the usual nanoid function. But since that would be a breaking change, perhaps this change would be easier to introduce?

let customAlphabet = (alphabet, defaultSize = 21) => {
  return (size = defaultSize) => {

So I started making the change, but I was unsure how to change all of the files. So I figured I'll just ask here, especially since there seems to be an extreme amount of attention paid to the resulting size of the function. 🤷

Thanks!

ai commented 2 years ago

I like the idea, but we also need to update Size Limit config (to see the price of these changes) and add tests for extra feature.

stefansundin commented 2 years ago

Ok, I am not exactly sure how to do that. If you prefer, feel free to take over this PR (branch allows edits by maintainers).

ai commented 2 years ago

If you prefer, feel free to take over this PR (branch allows edits by maintainers).

It is not really polite to ask maintainer to finish work.

In modern open source ecosystem there is too many pressure on maintainers.

But I can provide you a guide how to finish this PR.

  1. Install dependencies by pnpm install
  2. Run tests by pnpm test. Right now there is an error in your changes (size is not defined), you need to fix it first.
  3. Edit test/index.test.js and add test that generator(size) change the size of ID.
  4. Back-port changes to async/index.js and add tests to test/async.test.js.
  5. Change README.md (I will back-port changes to Russian version).
  6. Run pnpx size-limit, change size-limit section in package.json to reflect size changes.

If you do not have a time, we can always close PR and issue.

stefansundin commented 2 years ago

Thanks for the tips, it helped. I think I got things into a good shape, please let me know if there's anything else that you think is missing.

Looks like the benchmark failed in the CI, but I was able to run it fine myself.

ai commented 2 years ago

Looks good, thanks :+1:

Seems like benchmark fail is some CI bug. It is on me.

I will try to accept and release in on Monday/Tuesday.

ai commented 2 years ago

Thanks. Released in Nano ID 3.3.

jasikpark commented 2 years ago

Oh, this is a nice change - I had generated a closure around customAlphabet to provide the same variable size that nanoid allows