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

chore: QOL improvements #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robhanlon22
Copy link

@robhanlon22 robhanlon22 commented Dec 11, 2023

  • Move default blocklist to its own file so class Sqids is visible upon opening
  • Export minAlphabetLength, minLengthLimit, and maxValue so they're available to downstream consumers
  • Enforce Required<SqidsOptions> type on defaultOptions

@robhanlon22 robhanlon22 changed the title QOL improvements chore: QOL improvements Dec 11, 2023
- Move default blocklist to its own file so `class Sqids` is visible
  upon opening
- Export `minAlphabetLength`, `minLengthLimit`, and `maxValue` so
  they're available to downstream consumers
- Enforce `Required<SqidsOptions>` type on defaultOptions
@4kimov
Copy link
Member

4kimov commented Dec 12, 2023

@robhanlon22 Thanks for the PR. Two questions for you:

  1. Shouldn't newly exported constants be screaming snake case since that's typically the convention for hardcoded values?
  2. I do like the blocklist moved to a separate file, it's cleaner. Did you check if building the package worked nicely with it being moved? For some reason last time I couldn't get it to package properly.

@robhanlon22
Copy link
Author

  1. Shouldn't newly exported constants be screaming snake case since that's typically the convention for hardcoded values?

Sure, I can do that. I was following the convention of defaultOptions here, but I can change them. I'll also alias defaultOptions to DEFAULT_OPTIONS.

  1. I do like the blocklist moved to a separate file, it's cleaner. Did you check if building the package worked nicely with it being moved? For some reason last time I couldn't get it to package properly.

Sure, I'll try it out, and see if anything needs tweaking.

@4kimov
Copy link
Member

4kimov commented Dec 13, 2023

@robhanlon22 Cool, thank you.

Regarding naming convention, I always thought this approach was reasonable for JS: https://stackoverflow.com/a/49537197

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.

2 participants