feat!: support tree-shaking

[Shinigami92]

BREAKING CHANGE: importing of faker changes

[netlify]

✔️ 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]

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'

[Shinigami92]

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

[Shinigami92]

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

[Shinigami92]

This is the most important line of this PR 🙂

[Shinigami92]

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]

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]

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.

[clarkerican]

Looks good to me!

[ejcheng]

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.

[ejcheng]

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

[Shinigami92]

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

[Shinigami92]

We need to add export default faker; here

[JessicaSachs]

After the export default line is in, this lgtm