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

docs(usage): type definitions aren't strictly necessary #2840

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

fzn0x
Copy link
Contributor

@fzn0x fzn0x commented Apr 21, 2024

type definitions aren't strictly necessary here

type definitions aren't strictly necessary here
@fzn0x fzn0x requested a review from a team as a code owner April 21, 2024 16:50
Copy link

netlify bot commented Apr 21, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 22b4014
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/662675db58f59800088bc238
😎 Deploy Preview https://deploy-preview-2840.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (b754dc6) to head (22b4014).

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #2840    +/-   ##
========================================
  Coverage   99.95%   99.96%            
========================================
  Files        2973     2973            
  Lines      212613   212613            
  Branches      943      601   -342     
========================================
+ Hits       212510   212529    +19     
+ Misses        103       84    -19     

see 2 files with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Apr 21, 2024

type definitions aren't strictly necessary here

They might not be necessary there, but we have them there to reduce the cognitive load from the reader to guess what the method/field is supposed to do/return.

See also:

What is your idea/intention with removing those (aka Why)?

@ST-DDT ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: awaiting more info Additional information are requested labels Apr 21, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Apr 21, 2024
@fzn0x
Copy link
Contributor Author

fzn0x commented Apr 21, 2024

type definitions aren't strictly necessary here

They might not be necessary there, but we have them there to reduce the cognitive load from the reader to guess what the method/field is supposed to do/return.

See also:

What is your idea/intention with removing those (aka Why)?

Typings still included without specifying Users.
Screenshot 2024-04-22 003654

@fzn0x
Copy link
Contributor Author

fzn0x commented Apr 21, 2024

Improve the example for the Users type definition, or remove it altogether to allow users to explore the usage independently without encountering errors after choosing ESM or CJS.

@ST-DDT ST-DDT removed the s: awaiting more info Additional information are requested label Apr 21, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Apr 21, 2024

So the issue here is that the example on the readme uses a type that isnt specified anywhere on that page.

@ST-DDT ST-DDT requested review from a team April 21, 2024 18:28
@matthewmayer
Copy link
Contributor

Maybe remove "export" too? If it's just a standalone example.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 21, 2024

Maybe remove "export" too? If it's just a standalone example.

I'm undecided on that. Maybe a little bit on the keep it part because I hope this pushes the users to reuse their faker methods and it slightly catches more attention.

@ST-DDT ST-DDT merged commit a1b9ffd into faker-js:next Apr 22, 2024
20 checks passed
@fzn0x fzn0x deleted the patch-1 branch April 23, 2024 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants