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.

Sign up for GitHub

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

Jump to bottom

customAlphabet in nanoid/async/index.browser is not actually async #228

Closed
LoneRifle opened this issue Jul 24, 2020 · 2 comments · Fixed by #229
Closed

customAlphabet in nanoid/async/index.browser is not actually async #228

LoneRifle opened this issue Jul 24, 2020 · 2 comments · Fixed by #229

Comments

@LoneRifle
Copy link
Contributor

LoneRifle commented Jul 24, 2020 •
edited

Visual comparison of customAlphabet() between /index.browser.js and /async/index.browser.js suggests that the async version is really synchronous. This should either be changed to at least return a Promise, or documented.
@LoneRifle LoneRifle changed the title customAlphabet in nanoid/async is not actually async customAlphabet in nanoid/async/index.browser is not actually async Jul 24, 2020
@ai
Copy link
Owner

ai commented Jul 24, 2020

Yeap, it is sync, since browser do not have async API. We have it just for compatibility.

I like the idea to document it.

Can you send PR?

LoneRifle added a commit to opengovsg/GoGovSG that referenced this issue Jul 24, 2020
@LoneRifle
fix(nanoid): generate => customAlphabet
1aa425d 
Replace `generate()` in v2 with `customAlphabet()` in v3,
so that we can more easily adopt future versions of nanoid

- map `generateShortUrl` directly to `customAlphabet()`
- ai/nanoid#228 suggests that `customAlphabet()` is not async, so
  make `generateShortUrl()` synchronous, and change CreateLinkForm
  to account for this

This is a follow-up commit for dependabot
@LoneRifle
Copy link
Contributor Author

LoneRifle commented Jul 24, 2020 •
edited

I would love to, though I would like to raise the following to clarify the issue, for your consideration:

generate() in v2 was available in async form (by delegating to format(), which returns a Promise) for browsers, and most major browsers have at least some support for Promises. Further, nanoid() and random() in async/index.browser make at least one use of Promise.resolve().

I've filed #229 to address this, in light of the above.

@LoneRifle LoneRifle mentioned this issue Jul 24, 2020
Merged
@ai ai closed this as completed in #229 Jul 27, 2020
xming13 pushed a commit to xming13/GoGovSG that referenced this issue Aug 21, 2020
@LoneRifle @xming13
fix(nanoid): generate => customAlphabet
b79fce5 
Replace `generate()` in v2 with `customAlphabet()` in v3,
so that we can more easily adopt future versions of nanoid

- map `generateShortUrl` directly to `customAlphabet()`
- ai/nanoid#228 suggests that `customAlphabet()` is not async, so
  make `generateShortUrl()` synchronous, and change CreateLinkForm
  to account for this

This is a follow-up commit for dependabot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

async/customAlphabet - ensure browser version returns Promise
2 participants
@ai @LoneRifle