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

feat!: support tree-shaking #152

Merged
merged 2 commits into from
Jan 18, 2022
Merged

feat!: support tree-shaking #152

merged 2 commits into from
Jan 18, 2022

Conversation

Shinigami92
Copy link
Member

BREAKING CHANGE: importing of faker changes

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

netlify bot commented Jan 15, 2022

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

🔨 Explore the source changes: 0dd1f74

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

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

@Shinigami92 Shinigami92 requested a review from a team January 15, 2022 12:14
@Shinigami92
Copy link
Member Author

Shinigami92 commented Jan 15, 2022

when we merge this, we need to write a migration guide or at least provide a RegExp replace pattern for out users

legacy js

const faker = require('faker')

// to

const { faker } = require('@faker-js/faker')
// or
const faker = require('@faker-js/faker').faker

modern js

import faker from 'faker'
// or
import * as faker from 'faker'

// to

import { faker } from '@faker-js/faker'

// Using the helper function arr, randomly sized collections of elements are produced in the document.

var faker = require('../../lib').faker;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the important line in this file.
I didn't touched the rest of this file, just auto-prettier on save.

@@ -1,19 +1,26 @@
var fs = require('fs');

var faker = require('../../index');

var faker = require('../../lib').faker;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the important line in this file.
I didn't touched the rest of this file, just auto-prettier on save.

@@ -22,7 +22,7 @@
"url": "https://github.com/faker-js/faker.git"
},
"license": "MIT",
"main": "index.js",
"main": "lib/index.js",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most important line of this PR 🙂

@Shinigami92
Copy link
Member Author

This could affect how the browser version is used 🤔
I'm not sure but do we need something to do like this in src/index.ts?:

if (typeof window === 'object') {
  window.faker = faker;
}

@damienwebdev
Copy link
Member

We need an integration test running in CI to validate both Node behavior and browser behavior of the legacy scenario before I think we can merge this. I also think that we move this to v7 to give our dependents some space to focus on their work rather than a breaking change.

@Shinigami92
Copy link
Member Author

We need an integration test running in CI to validate both Node behavior and browser behavior of the legacy scenario before I think we can merge this. I also think that we move this to v7 to give our dependents some space to focus on their work rather than a breaking change.

To me it's not such a critical change + I would like to do it now, cause the folks that migrate to us need to rewrite the import statements anyways.
So instead of doing it twice, they just need it to do it once.

@prisis prisis added this to the v6.0.0 milestone Jan 16, 2022
clarkerican
clarkerican previously approved these changes Jan 17, 2022
Copy link
Contributor

@clarkerican clarkerican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Shinigami92 Shinigami92 mentioned this pull request Jan 17, 2022
@Shinigami92 Shinigami92 requested review from a team January 17, 2022 15:50
@import-brain
Copy link
Member

We need an integration test running in CI to validate both Node behavior and browser behavior of the legacy scenario before I think we can merge this. I also think that we move this to v7 to give our dependents some space to focus on their work rather than a breaking change.

By semantic versioning, this would be a "next major version" change anyway, because it would be a possible breaking change.

@import-brain
Copy link
Member

I'll mark this as needs tests because we should probably write tests for this to make sure it works before we merge it.

@import-brain import-brain added the needs test More tests are needed label Jan 17, 2022
MilosPaunovic
MilosPaunovic previously approved these changes Jan 18, 2022
@Shinigami92
Copy link
Member Author

Shinigami92 commented Jan 18, 2022

We need an integration test running in CI to validate both Node behavior and browser behavior of the legacy scenario before I think we can merge this. I also think that we move this to v7 to give our dependents some space to focus on their work rather than a breaking change.

By semantic versioning, this would be a "next major version" change anyway, because it would be a possible breaking change.

v6 is currently the next major change, we just released alphas, so nothing wrong with that from my POV

I'll mark this as needs tests because we should probably write tests for this to make sure it works before we merge it.

Could you explain what you mean what test do we need? Currently all ~20k are passing and I'm very happy that we have so much.
This PR does not affect the runtime behavior at all, just improve how you import it.

I may provide a regex-pattern and/or a sed-command so folks can easily update.
But IMO we already do need these patterns right now, cause the import already changed from faker to @faker-js/faker.

Edit: created an issue for that #212

// since we are requiring the top level of faker, load all locales by default
export const faker: Faker = new Faker({
locales: require('./locales'),
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add export default faker; here

BREAKING CHANGE: importing of faker changes
JessicaSachs
JessicaSachs previously approved these changes Jan 18, 2022
Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the export default line is in, this lgtm

@Shinigami92 Shinigami92 requested review from MilosPaunovic, prisis and a team January 18, 2022 18:46
@Shinigami92 Shinigami92 merged commit ccf119d into main Jan 18, 2022
@Shinigami92 Shinigami92 deleted the support-tree-shaking branch January 18, 2022 19:10
Shinigami92 added a commit to MohdImran001/faker that referenced this pull request Jan 18, 2022
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test More tests are needed p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants