-
-
Notifications
You must be signed in to change notification settings - Fork 883
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
feat(string): adds support for generating ULID #2524
base: next
Are you sure you want to change the base?
feat(string): adds support for generating ULID #2524
Conversation
Update: improved the description |
Thank you for your contribution. Since this PR doesn't have a relatet issue, please understand that we might need some time to decide on the usability of this feature. Thank you for your patience. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2524 +/- ##
==========================================
- Coverage 99.94% 99.94% -0.01%
==========================================
Files 2958 2958
Lines 213715 213730 +15
Branches 603 952 +349
==========================================
+ Hits 213595 213602 +7
- Misses 120 124 +4
- Partials 0 4 +4
|
Team Decision
The PR description contains a script that can be used as a workaround. |
Can we add a matching issue so it can collect upvotes? |
didn't find any issues related to this, but I would like to state my support for this feature! |
While that might be true, I would be in favor to build it differently and make use of |
Added #2648 to allow for collecting upvotes (we normally try to get 10 upvotes to indicate sufficient interest) and further promote discussion |
# Conflicts: # test/modules/__snapshots__/string.spec.ts.snap
@brunocleite Please fix the failing tests. Probably using |
…ation' into feat/adds-support-for-ulid-generation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/modules/string/index.ts
Outdated
*/ | ||
ulid(): string { | ||
return ( | ||
this.fromCharacters('012', 1) + |
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.
perhaps add a comment here that ULIDs starting with 0, 1 or 2 cover the years 1970-5314
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've updated the PR with a new solution proposal addressing the suggestions by @Shinigami92.
It should now generate the timestamp part of the ULID from refDate
.
…ation' into feat/adds-support-for-ulid-generation
Thanks. I'm looking at the examples on I've done a proposal implementation and updated this PR to achieve this, though, I have a couple questions:
|
toDate is currently being rewritten as part of #2757 so probably makes sense to wait for that to be merged first. |
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.
Please note that this is still waiting for community interest.
const { refDate } = options; | ||
let date = refDate; | ||
|
||
if (date == null) { |
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 think the comment was more about adding a comment to explain the magic numbers than to add an refDate parameter.
Nevertheless instead of copying the implementation, we should export the toDate function as internal helper and reuse it here.
There is another type of unique identifier other than UUID that is called ULID Universally Unique Lexicographically Sortable Identifier.
This type of identifier has the benefit of maintaining a sortable order and is useful as primary keys on databases (mainly NoSQL) to get free date-time sorting out of the box. More information here
ULID is a 26 characters string which the first 10 characters are the timestamp as millis from the epoch and the other 16 are the randomness.
The allowed characters are from
Crockford's Base32
which excludes letters I, L, and O to avoid confusion with digits.On the code excerpt above it is using the first character as [012] because that would already generate dates up to year 5314 which is more than enough. To confirm this behavior you can go here and try using
2ZZZZZZZZZWZYF0EJ600G2SZHA
as an ULID.