fix: allow to override only root mock #1338
Conversation
|
I love your ideas of Yarn 4 and cleaning up the tests as I do find it quite overwhelming. Right now it looks like there is a test failure though? |
|
I tried failing test in main and it also fails there break doesn't feel related to my changes 🤔 |
|
Feels like |
|
You could be right it might not be related to your change. |
|
I added a fix for |
| @@ -37,8 +37,6 @@ components: | |||
| ParentType: | |||
| type: object | |||
| properties: | |||
| value: | |||
There was a problem hiding this comment.
Could you please definition in more detail why this description is no longer necessary?
There was a problem hiding this comment.
So in main generated code looks like this:
Notice multiple ...overrideResponse and overrideResponse is of type any
export const getGetPolymorphicResponseResponseMock = (
overrideResponse: any = {},
): GetPolymorphicResponse200 =>
faker.helpers.arrayElement([
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
...overrideResponse,
...overrideResponse,
},
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
...overrideResponse,
...overrideResponse,
},
]);as this PR stoped overriding nested data in generated code same part looks like this:
export const getGetPolymorphicResponseResponseMock =
(): GetPolymorphicResponse200 =>
faker.helpers.arrayElement([
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
},
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
},
]);as spread of any is gone TS starts to complain because GetPolymorphicResponse200 is impossible type
I don't know at which point things got broken because looking at original polymorphic.yaml in swagger editor
value key should be ignored from ParentType
Would make sense to do a followup PR rather than try to fix things in this PR
There was a problem hiding this comment.
Thank you for letting me know so politely.
In other words, the mock was originally broken, and by expanding overrideResponse, the error was suppressed and it worked, right?
There was a problem hiding this comment.
As you said, the actual failure occurred at a different time, but it is true that what was previously working will no longer work as a result of taking this action.
This will result in the bug being included in the next release unless you follow up after merging this PR.
It doesn't necessarily have to be the same PR, and I think it's better to separate them, but is it possible to follow up at the same time?
There was a problem hiding this comment.
One possible solution could be to type the different possibilities, like this:
// GetPolymorphicResponse200 = DescendantOne | DescendantTwo
faker.helpers.arrayElement([
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
} as DescendantOne,
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, undefined]),
} as DescendantTwo,
]);One improvement here could maybe also be to add the "overridden" type to the array as well? Something like
faker.helpers.arrayElement([
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, faker.datatype.boolean(), undefined]),
} as DescendantOne,
{
key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
undefined,
]),
value: faker.helpers.arrayElement([{}, faker.word.sample(), undefined]),
} as DescendantTwo,
]);There was a problem hiding this comment.
feels like I opened can of worms 😅
Issue is not in faker code, it's GetPolymorphicResponse200 that's broken.
that type cannot be used even outside of faker.
I don't mind to investigate the case but let's consider this
generated types are broken already.
as no-one reported broken types probably it means no-one has such use case in their project.
hence I would argue that we're not actually breaking anyones code.
I'm on vacation next week so any update from my side will arrive with delay if I don't dig up anything today.
There was a problem hiding this comment.
So I traveled back to commit where polymorphic.yaml was added
faker code were not typed at that time and generated types behave as shown here
export const getGetPolymorphicResponseMock = () =>
faker.helpers.arrayElement([
{
value: faker.helpers.arrayElement([{}, undefined]),
key: faker.helpers.arrayElement([faker.random.word(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING']),
undefined,
]),
},
{
value: faker.helpers.arrayElement([{}, undefined]),
key: faker.helpers.arrayElement([faker.random.word(), undefined]),
type: faker.helpers.arrayElement([
faker.helpers.arrayElement(['BOOLEAN', 'STRING']),
undefined,
]),
},
]);I suspect Orval never really supported situation that was created in polymorphic.yaml
According to Official Swagger docs Orval is generating correct types and polymorphic.yaml is broken.
There is no hierarchy in allOf hence data should comply with all schemas thus I don't think ignoring value: Record from ParentType is a way forward.
There was a problem hiding this comment.
Thank you for investigating.
I understand that Polymorphic is not working and the types being generated are flawed, and the problem manifests itself not only in mocks. What I was concerned about was breaking a working state in an unintentional way.
However, even if we only take care of the mock with this PR, the fundamental problem will not be resolved and it is not essential, so we agree to treat it as a separate issue in favor of the original purpose of this PR.
There was a problem hiding this comment.
@soartec-lab generated types are not flawed.
Quite contrary, they show just what polymorphic.yaml intends to.
But polymorphic.yaml is flawed as a test example.
value key should not be part of ParentType schema
Hence if we agree to my change (removal of value from ParentType) I would consider test data fixed and case closed
There was a problem hiding this comment.
I see, I understood that there is no problem with the generated type, but the problem is with the OpenAPI definition. If this change causes problems for some users, they will need to modify their own OpenAPI definitions.
I was able to understand correctly. And I think that's correct.
karlismelderis-mckinsey
force-pushed
the
fix/faker-overrides
branch
3 times, most recently
from
0b207b8
to
dc2a1ab
Compare
karlismelderis-mckinsey
requested review from
melloware,
Maxim-Mazurok and
soartec-lab
There was a problem hiding this comment.
@karlismelderis-mckinsey
I commented, could you please check it?
|
Yes that is ok |
I'm back from vacation 🌴 To unblock our work can we please merge and release this fix and then focus on "DeepPartial" PR? |
|
@karlismelderis-mckinsey |
|
Hi @Maxim-Mazurok, I wanna merge this but you are in "request change" status. Could you please check this again? |
I had a very brief look and dismissed my review, unfortunately don't have a capacity for a full review now |
karlismelderis-mckinsey
force-pushed
the
fix/faker-overrides
branch
from
e7a6429
to
9182fdf
Compare
Let's merge it before any new conflicts arrive 😉 |
|
@karlismelderis-mckinsey |
So I know how to plan our time, when do we expect to have next release? |
|
I think it will be soon. Do you have plan to next release soon? |
Status
READY
Description
Allow to override only root of mocked data and type overrideResponse argument as
Partial<Type>instead ofanyI also regenerated all react hook samples and rerun format for all repo hence there are quite some unrelated diff
Fix #1335
I'll try to find time to do two more follow up PRs:
petstore.yamlfor all samples