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.

Sign up for GitHub

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

Jump to bottom

fix: ts support for locales #169

Merged
merged 12 commits into from
Jan 19, 2022
Merged

fix: ts support for locales #169

merged 12 commits into from
Jan 19, 2022

Conversation

Shinigami92
Copy link
Member

No description provided.

@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Jan 16, 2022
@Shinigami92 Shinigami92 self-assigned this Jan 16, 2022
@netlify
Copy link

netlify bot commented Jan 16, 2022 •
edited
Loading

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: 7416f68

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e81aa34efdfa000788afd8

😎 Browse the preview: https://deploy-preview-169--vigilant-wescoff-04e480.netlify.app

@Shinigami92 Shinigami92 linked an issue Jan 16, 2022 that may be closed by this pull request
Provide TypeScript support for locales #167
Closed
@Shinigami92 Shinigami92 changed the title chore: define type for locales fix: ts support for locales Jan 16, 2022
@Shinigami92
Copy link
Member Author

Oh... I found out that there a locale (without s at the end) that exports for each locale one faker instance 👀
But these are surely currently not typed or covered.

  1. We should move them into src for now so we can migrate them also to TS
  2. As already planned, we should think about a better strategy to load locales

@prisis prisis added this to the v6.0.0 milestone Jan 16, 2022
@Shinigami92
Copy link
Member Author

You need to review this PR with the commits one by one, cause there is at least one commit that has 2x1700 file changes and is not reviewable

@Shinigami92 Shinigami92 marked this pull request as ready for review January 16, 2022 15:56
@Shinigami92 Shinigami92 mentioned this pull request Jan 18, 2022
Closed
@Shinigami92 Shinigami92 mentioned this pull request Jan 18, 2022
Closed
@Shinigami92
Copy link
Member Author

Shinigami92 commented Jan 18, 2022 •
edited
Loading

This is now rebased and can be approved

Nevermind, tests are failing 👀

@Shinigami92
Copy link
Member Author

Needed to use curly brace imports within the locale files, cause the default import is now not the Faker constructor but the default faker instance
This affects only internal behavior 🙂
The external behavior will remain the same, especially after #217

@Shinigami92 Shinigami92 mentioned this pull request Jan 19, 2022
Closed
@Shinigami92 Shinigami92 merged commit 61d0296 into main Jan 19, 2022
@Shinigami92 Shinigami92 deleted the locales-ts-support branch January 19, 2022 15:58
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
@Shinigami92 @pkuczynski
fix: ts support for locales (faker-js#169)
c75257c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JessicaSachs JessicaSachs JessicaSachs approved these changes

@prisis prisis prisis approved these changes

@damienwebdev damienwebdev Awaiting requested review from damienwebdev

@import-brain import-brain Awaiting requested review from import-brain

Assignees

@Shinigami92 Shinigami92

Labels
p: 1-normal Nothing urgent
Projects
None yet
Milestone
v6.0 - Ensure Project Foundation
Development

Successfully merging this pull request may close these issues.

Provide TypeScript support for locales
3 participants
@Shinigami92 @prisis @JessicaSachs