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

refactor!: get rid of export = #849

Merged
merged 4 commits into from
May 3, 2022
Merged

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 20, 2022

This PR is an requirement for #848

@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Apr 20, 2022
@Shinigami92 Shinigami92 added this to the v7 - Next Major milestone Apr 20, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 20, 2022
@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Apr 20, 2022
@Shinigami92
Copy link
Member Author

We need to decide if we want to introduce a breaking change and users need to use require('@faker-js/faker/locale/en').default or if we want back somehow the current behavior without .default.
It looks like we are trapped in a legacy design issue with that.

@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #849 (5e73fc3) into main (a2da7c4) will increase coverage by 0.51%.
The diff coverage is 100.00%.

❗ Current head 5e73fc3 differs from pull request most recent head 2751fcc. Consider uploading reports for the commit 2751fcc to get more accurate results

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
+ Coverage   99.13%   99.64%   +0.51%     
==========================================
  Files        1958     1958              
  Lines      210793   210679     -114     
  Branches      906      907       +1     
==========================================
+ Hits       208964   209931     +967     
+ Misses       1753      729    -1024     
+ Partials       76       19      -57     
Impacted Files Coverage Δ
src/locale/af_ZA.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/ar.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/az.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/cz.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/de.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/de_AT.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/de_CH.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/el.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/en.ts 100.00% <100.00%> (+100.00%) ⬆️
src/locale/en_AU.ts 100.00% <100.00%> (+100.00%) ⬆️
... and 104 more

@Shinigami92
Copy link
Member Author

An alternative could be to switch totally to export { faker };
and then users need to use

// js
const fakerEn = require('@faker-js/faker/locale/en').faker
const { faker: fakerEn } = require('@faker-js/faker/locale/en')

// ts
const { faker } from '@faker-js/faker/locale/en'
const { faker as fakerEn } from '@faker-js/faker/locale/en'

This is more verbose but also has the benefit of just swapping out from '@faker-js/faker' => '@faker-js/faker/locale/en' 🤔

@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2022

I like that alternative proposal.
I assume most of the time you only need a specific language anyway.

@Shinigami92
Copy link
Member Author

I like that alternative proposal. I assume most of the time you only need a specific language anyway.

@prisis I would like to hear your vote for that, if you also accept, I will refactor it to that

@prisis
Copy link
Member

prisis commented Apr 21, 2022

Im voting for the alternative proposal too, @Shinigami92

@Shinigami92 Shinigami92 force-pushed the refactor-get-rid-of-export-equal branch from cb28b85 to dcbf3e9 Compare April 21, 2022 14:53
@Shinigami92 Shinigami92 added do NOT merge yet Do not merge this PR into the target branch yet and removed s: needs decision Needs team/maintainer decision labels Apr 21, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review April 21, 2022 14:54
@Shinigami92 Shinigami92 requested a review from a team April 21, 2022 14:54
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 21, 2022 14:54
@Shinigami92 Shinigami92 requested review from ST-DDT and a team April 21, 2022 18:29
ST-DDT
ST-DDT previously approved these changes Apr 21, 2022
scripts/bundle.ts Outdated Show resolved Hide resolved
pkuczynski
pkuczynski previously approved these changes Apr 21, 2022
@Shinigami92 Shinigami92 force-pushed the refactor-get-rid-of-export-equal branch from d72331c to 9ff03d7 Compare May 3, 2022 13:55
@Shinigami92 Shinigami92 removed the do NOT merge yet Do not merge this PR into the target branch yet label May 3, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 3, 2022

Doesn't this contain the changes from?
#850

@Shinigami92
Copy link
Member Author

Doesn't this contain the changes from? #850

Not anymore 😉

@Shinigami92 Shinigami92 enabled auto-merge (squash) May 3, 2022 15:01
@Shinigami92 Shinigami92 merged commit 20fbeaf into main May 3, 2022
@Shinigami92 Shinigami92 deleted the refactor-get-rid-of-export-equal branch May 3, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants