-
Notifications
You must be signed in to change notification settings - Fork 49
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
add support for useImplementingTypes #125
add support for useImplementingTypes #125
Conversation
src/index.ts
Outdated
return `relationshipsToOmit.has('${casedName}') ? {} as ${casedName} : ${toMockName( | ||
name, | ||
casedName, | ||
opts.prefix, | ||
)}({}, relationshipsToOmit)`; | ||
} else { | ||
return handleValueGeneration(opts, null, () => `${toMockName(name, casedName, opts.prefix)}()`); | ||
return `${toMockName(name, casedName, opts.prefix)}()`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, this handleValueGeneration
was added this week, are you sure it needs to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, sorry about that.
I will fix it.
do i need to use "handleValueGeneration" in getNamedImplementType func? @ardeois
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes probably, handleValueGeneration
is necessary when you use config fieldGeneration
Here is a unit test example. You should add one to make sure it works with implementing types
it('uses per field generation if field name matches', async () => { | |
const result = await plugin(testSchema, [], { | |
...config, | |
fieldGeneration: { | |
A: { email: 'internet.email' }, | |
}, | |
}); | |
expect(result).toBeDefined(); | |
// Custom generation in type A | |
expect(result).toContain( | |
"email: overrides && overrides.hasOwnProperty('email') ? overrides.email! : faker['internet']['email']()", | |
); | |
// Original generation in type B (unchanged) | |
expect(result).toContain( | |
"email: overrides && overrides.hasOwnProperty('email') ? overrides.email! : faker['lorem']['sentence'](),", | |
); | |
expect(result).toMatchSnapshot(); | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is related, nor is it necessary.
if someone using fieldGeneration, why use useImplementingTypes as well?
This is contrary to the idea. @ardeois
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example:
const result = await plugin(testSchema, [], {
...config,
fieldGeneration: {
A: { config: 'internet.config' },
},
});
expect(result).toContain(
"config: overrides && overrides.hasOwnProperty('config') ? overrides.config! : internet.config() || internet.config(),",
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've fix it.
Now if the prop useimplementingtypes is true and the fieldgeneration is not empty, the code will ignore the implant types.. i added test about that @ardeois
Thanks for implementing this @maordaniel I've got a few comments and the unit tests are failing |
Thank you for reviewing @ardeois |
@maordaniel tests related to your code are now failing |
84f818e
to
8f12972
Compare
Fixed and i added one more test @ardeois |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review, I got quite busy these last few days.
I tried to do an in depth review and found potential issues.
Let me know what you think
Thanks again for the help !
Hi Ardeois, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ! thanks for the updates
Thanks a lot, I updated readme file and the snapshot for the test, pls approve this again. |
Hi @ardeois , Thanks |
Sorry about that @maordaniel I forgot |
Thanks a lot! 🙏🏻 @ardeois |
Add support for useImplementingTypes GraphQL codegen configuration.