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

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 •
edited

✔️ 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 •
edited

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
Shinigami92 commented Jan 15, 2022
View reviewed changes
examples/node/generateMultiLevelMultiLocaleJSON.js
// 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.

examples/node/generateSet.js
@@ -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.

package.json
@@ -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
View reviewed changes
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
Closed
@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
View reviewed changes
@Shinigami92
Copy link
Member Author

Shinigami92 commented Jan 18, 2022 •
edited

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 Shinigami92 mentioned this pull request Jan 18, 2022
Closed
Shinigami92
Shinigami92 commented Jan 18, 2022
View reviewed changes
src/index.ts
// 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

@Shinigami92
feat: support tree-shaking
22e4244 
BREAKING CHANGE: importing of faker changes
JessicaSachs
JessicaSachs previously approved these changes Jan 18, 2022
View reviewed changes
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
@Shinigami92
feat!: support tree-shaking (faker-js#152)
236a66d
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
@Shinigami92 @pkuczynski
feat!: support tree-shaking (faker-js#152)
81d7d5e
@ST-DDT ST-DDT mentioned this pull request Feb 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prisis prisis prisis approved these changes

@JessicaSachs JessicaSachs JessicaSachs approved these changes

@MilosPaunovic MilosPaunovic MilosPaunovic approved these changes

@clarkerican clarkerican Awaiting requested review from clarkerican

Assignees

@Shinigami92 Shinigami92

Labels
needs test More tests are needed p: 1-normal Nothing urgent
Projects
None yet
Milestone
v6.0 - Ensure Project Foundation
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Shinigami92 @damienwebdev @import-brain @prisis @JessicaSachs @clarkerican @MilosPaunovic