feat(deps): switch to faker from casual #92
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ TODO:
📝 memo:
Check compatibility
|
Because faker almost completed migrating to TypeScript so that DefinitelyTyped no longer needs to maintain its external [@types/faker](https://www.npmjs.com/package/@types/faker) package. ref: https://fakerjs.dev/about/announcements/2022-01-14.html
Since the seed value changes with the switching from casual to faker, the snapshots needs to be updated. I confirm that the format does not change significantly, despite the differences.
As shown in the following code, the result changes even if the seed is added:
```
> const { faker } = require("@faker-js/faker")
undefined
> faker.seed(0)
0
> const a = faker.date.past().toISOString()
undefined
> a
'2022-03-08T21:45:15.158Z'
> faker.seed(0)
0
> const b = faker.date.past().toISOString()
undefined
> b
'2022-03-08T21:45:30.411Z'
> a === b
false
```
This is because the default refDate passed in the second argument of faker.date.past() is now, so we can fix it at a specific date.
ref: faker-js/faker#859
MH4GF
force-pushed
the
switch-to-faker-from-casual
branch
from
2026f75
to
3bb083b
Compare
I grep'd and changed only the part I replaced this PR to faker.
ardeois
reviewed
Sep 26, 2022
There was a problem hiding this comment.
Thanks for the implemetation!
I have a few comments about breaking changed, I'm not a fan of using casual and faker at the same time, especially because the generated output could have breaking change
I would see few options to reduce the breaking changes for users:
- Use faker all the way. This includes generation when
dynamicValuesistrueorfalse. As the output will be different already, I think we can break the scalar option too. This option is the most user's impacting but at least we don't have legacy code - Use faker only when
dynamicValuesistrue. We keep casual for the rest of the code, so whendynamicValues == falsewe call casual, and it won't change the output of generated mocks. It comes with the downside of using 2 libraries for value generation and could be confusing when users start usingdynamicValues. This would however be confusing for custom scalars - Have an option to select the generator library. We could have an option like
generatorLibrary: 'faker' | 'casual'(default:casual). This way, we can introduce the changes smoothly. Users not touchinggeneratorLibrarywon't have any breaking change, but those wanting last features can benefit fromfaker. It would also impact custom scalars. Then for the next major version, we can deprecatecasualand change the default fromcasualtofaker. This leaves some time to developers to upgrade
I kind of like option 3 even if it's more job. I think we could create a helper file (or class) to simplify the code. This class would generate the values or function calls for us based on the generatorLibrary option
Something like this:
class ValueGenerator {
constructor(private options: {dynamicValues: boolean, generatorLibrary: 'casual' | 'faker'}) {}
word() {
if (this.options.generatorLibrary === 'faker') {
return this.options.dynamicValues ? `faker.lorem.word()` : `'${faker.lorem.word()}'`;
} else {
return this.options.dynamicValues ? `casual.word` : `'${causl.word}'`;
}
}
}| const name = opts.currentType.name.value; | ||
| const casedName = createNameConverter(opts.typenamesConvention, opts.transformUnderscore)(name); | ||
| switch (name) { | ||
| case 'String': | ||
| return opts.dynamicValues ? `casual.word` : `'${casual.word}'`; | ||
| return opts.dynamicValues ? `faker.lorem.word()` : `'${faker.lorem.word()}'`; |
There was a problem hiding this comment.
I wonder if we should call casual if dynamicValues is false ?
|
@ardeois Thank you for your thoughtful comments! As @JimmyPaolini points out here #90 (comment) , the current implementation has a problem that is no different from a destructive change because the output results are different. As a solution to this, I think that your suggested option 3 is a better choice. It makes sense that most users would be satisfied with casual and only those who want to use faker can choose it of their own volition. I'll try to add the generatorLibrary option after work! Also the ValueGenerator class you suggested looks good. |
|
Closing this PR as #93 cover it already with retro-compatible config to avoid breaking changes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #90
What
Switch casual with faker, which was used in the generation process of the mocked temporary values.
Context
casual dependents on the node API(e.g. fs) and cannot be made to work in the browser.
ref: #90
Problem
This library has the feature for custom generators to embed in casual property calls, and switching to faker would be a breaking change.
Since it is not good to force many users to migrate for the dynamicValues feature that few users use yet, I think it is better to migrate only the other mock functions on a temporary basis.