-
Notifications
You must be signed in to change notification settings - Fork 1
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
move to nanoid for tenant, user, apps! #275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if a change like this was documented better. For example referencing https://zelark.github.io/nano-id-cc/ to show what the probability of collision is.
@@ -83,6 +84,7 @@ export class AppsService { | |||
if (!tenants) return; | |||
const newApp = await this.prisma.client.app.create({ | |||
data: { | |||
id: nanoid(10), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any implications of 10 here? I'm not familiar with nanoid, so not sure what benefit this has over 9, or what would be gained by going to 11. This is the type of decision that is good to document so future devs know what's going on. I'd recommend making a constant since this is used in several places (2), and the constant can be named and commented appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://zelark.github.io/nano-id-cc/
10 character is strong enough to not repeat !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
No description provided.