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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Feb 14, 2022
Merged

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

merged 2 commits into from Feb 14, 2022

Conversation

stefansundin
Copy link
Contributor

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:

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

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

ai commented Feb 10, 2022

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

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

ai commented Feb 11, 2022

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

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

ai commented Feb 12, 2022

Looks good, thanks 馃憤

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

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

@ai ai merged commit 38f1888 into ai:main Feb 14, 2022
@ai
Copy link
Owner

ai commented Feb 14, 2022

Thanks. Released in Nano ID 3.3.

@jasikpark
Copy link

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants