-
-
Notifications
You must be signed in to change notification settings - Fork 920
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
refactor(helpers)!: remove v8 deprecated unique #2661
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2661 +/- ##
==========================================
- Coverage 99.55% 99.55% -0.01%
==========================================
Files 2817 2816 -1
Lines 251188 250918 -270
Branches 1117 707 -410
==========================================
- Hits 250074 249805 -269
+ Misses 1114 1084 -30
- Partials 0 29 +29
|
As mentioned earlier. I don't think we should remove unique from faker as it is a very useful feature when generating test data. EDIT: Reason: #2667 in combination with #2664 will likely invalidate the circumstances that the initial decision was made upon. Please vote this comment with
|
What about the possibility of taking an external replacement like https://www.npmjs.com/package/enforce-unique and moving it within the faker-js org, but as a separate package/repo? |
I need to say that I 100% dislike to bring up a long discussion we already had and even written down in our internal notes. As Matthew already wrote: there is another package that does the same if not better. |
Let's just make sure we have very good documentation explaining how to migrate from using the existing method to a third-party package. |
i went ahead and added some documentation and suggested alternatives direct on this branch. Feedback welcome. |
format docs fix code simplify code move the CJS example remove the unique section from usage.md and reference new page
4d6bbf6
to
ff38a0c
Compare
Thank you @matthewmayer LGTM 👍 |
Please dont rebase unless necessary as it makes reviews harder. |
This will also need a migration guide snippet. Hopefully it can be quite short now and just link to the unique guide page. |
added |
I published a new version of |
Co-authored-by: Eric Cheng <[email protected]>
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.
Approved as per old team decision so it's no longer hard blocked, but I still think it's wrong.
I owe you a favor 👍 |
extracted from #2608
referencing #1785
should be merged before starting #2335
sideEffects: false
topackage.json
#2335