feat: enhance overrideResponse assignment

[DevSDK]

Status

READY

Description

1. Apply loadsh/merge

According to the ECMAScript specification [1], The spread syntax does not guarantee deep assignment.

Because the properties will overwrite per each key by CreateDataProperty.

1. Let newDesc be the PropertyDescriptor { [[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true }.
2. Return ? O.[[DefineOwnProperty]](P, newDesc).

(Also Object.assign [2] - [[Set]] in this case operation.)

For example:

const override = { 
   data: {
          content: "John Doe"
   }
};


const result = { 
   data: {
      type: "NAME",
      content: "Seokho"
      // and the other properties mocked by fackerjs
   },
    ...override
};

The result will be:

{
   data: {
          content: "John Doe"
   }
}

The user expectation that presents given properties is only valid on 1depth of the result.

Therefore, apply loadsh/merge [3] to assign property recursively.

2. Deterministic array length by overrideResponse

The overrideResponse can contain an array property. Currently, the override target generates a random length of it.

The length of the generated array can be shorter than the length in overrideResponse. That possibly leads to unexpected overriding behavior.

For example:

  const generated = [
     {
        content: "Seokho",
        id: "test-id", 
       // and many other properties... include required type
     },
  ]
  // The second one can present a non-deterministic result 
  // between only one property and added other properties per each call.
  const mocked = merge(generated, [{ content: "John Doe"}, { content: "Apple" }])

  

Hence, get length information from the array of the given overrideResponse to ensure the length of the generated array is greater than the given parameter.

The result will be looks like:

   // ...
    {
      foo: Array.from(
        {
          length: Math.max(
            overrideResponse?.['foo']?.length ?? 0,
            faker.number.int({ min: 1, max: 10 }),
          ),
        },
        (_, i) => i + 1,
      ).map((_, index_0) =>  faker.helpers.arrayElement([
       // ...
            set: faker.helpers.arrayElement([
              {
                 testSet: Array.from(
                  {
                    length: Math.max(
                      overrideResponse?.['foo']?.[index_0]?.['set']?.['testSet']
                        ?.length ?? 0,
                      faker.number.int({ min: 1, max: 10 }),
                    ),
                  },
       // ...

[1] https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-runtime-semantics-propertydefinitionevaluation

[2] https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-object.assign

FYR: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#spread_in_object_literals

[3] https://lodash.com/docs/4.17.15#merge

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

• [ ] Tests
• [ ] Documentation
• [ ] Changelog Entry (unreleased)

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

> git pull --prune
> git checkout <branch>
> grunt jasmine

[DevSDK]

@melloware Oops sorry. I didn't expect the Pull request to close when I renamed the branch on Github.

[soartec-lab]

@DevSDK Thank you for made this PR, I'll review this.

[soartec-lab]

For example, there are some functions that can no longer be overridden, such as tests/generated/angular/tags-split/pets/pets.msw.ts. Can you check this?

Below are the previous functions:

export const getListPetsMock = (overrideResponse: any = {}): Pets => (Array.from({ length: faker.number.int({ min: 1, max: 10 }) }, (_, i) => i + 1).map(() => (faker.helpers.arrayElement([{breed: faker.helpers.arrayElement(['Labradoodle'] as const), cuteness: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse},{breed: faker.helpers.arrayElement(['Dachshund'] as const), length: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse},{petsRequested: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['cat'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse}]))))

export const getCreatePetsMock = (overrideResponse: any = {}): Pet => (faker.helpers.arrayElement([{breed: faker.helpers.arrayElement(['Labradoodle'] as const), cuteness: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse},{breed: faker.helpers.arrayElement(['Dachshund'] as const), length: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse},{petsRequested: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['cat'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse}]))

export const getShowPetByIdMock = (overrideResponse: any = {}): Pet => (faker.helpers.arrayElement([{breed: faker.helpers.arrayElement(['Labradoodle'] as const), cuteness: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse},{breed: faker.helpers.arrayElement(['Dachshund'] as const), length: faker.number.int({min: undefined, max: undefined}), ...overrideResponse,barksPerMinute: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['dog'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse},{petsRequested: faker.helpers.arrayElement([faker.number.int({min: undefined, max: undefined}), undefined]), type: faker.helpers.arrayElement(['cat'] as const), ...overrideResponse,'@id': faker.helpers.arrayElement([faker.word.sample(), undefined]), callingCode: faker.helpers.arrayElement([faker.helpers.arrayElement(['+33','+420','+33'] as const), undefined]), country: faker.helpers.arrayElement([faker.helpers.arrayElement(['People\'s Republic of China','Uruguay'] as const), undefined]), email: faker.helpers.arrayElement([faker.internet.email(), undefined]), id: faker.number.int({min: undefined, max: undefined}), name: faker.word.sample(), tag: faker.helpers.arrayElement([faker.word.sample(), undefined]), ...overrideResponse}]))

[anymaniax]

I am not sure that it’s a good idea to force the user to install lodash merge no?

[melloware]

I agree I am not a fan of forced dependencies.

[soartec-lab]

I don't want to increase the number of dependent libraries if possible. It is convenient because there are no dependent libraries, and lodash is a general-purpose library, which may be a reason for some users to avoid it.

[DevSDK]

Make sense. I respect that.

We can choose the alternative ways:

Implement recursive assignments like lodash.merge, and then

1. Insert on top of every .msw file This way requires some mechanism to inject only one function on top of the msw file. (Does a mechanism already exist to handle it?)
2. Or add like libs/ directory and import that. It needs to copy or generate a file that we want to import. (AFAIK, The /mock package is unable to handle this right?) Perhaps we should create an initializer on /mock or add another place other than /mock

Which way would you like prefer to approach?

Let me know prefer way and it would be great to advise me on some consideration points of each way if you don't mind.

[DevSDK]

@soartec-lab

Related: https://github.com/anymaniax/orval/pull/1199#issuecomment-1931733511

Yes. It has become unable to override.

Thanks for letting me know :).

Because it will be decided by whether the 'overrideResponse' string exists or not in value.

It happened by removing '...overrideResponse' string from the value string by removing the push statement in the object.js.

The simplest way to resolve this is adding /* overrideResponse */ comment right before place. That would work with the previous logic.

I assume a bit complex way will be to return overridable information from getMockDefinition() or store that information somewhere.

How about a simplest way? Or can you suggest fixing this case?

[soartec-lab]

@DevSDK I will think about both methods together. However, there are other things I would like to do, so I will deal with them in order.

[soartec-lab]

Regarding the override variable being lost, if it was implemented correctly without adding comments, the current string match judgment would work fine. And, alternative implementations of lodash, i may need to discuss more whether the ideas you want to improve are compatible with ours.

[DevSDK]

@soartec-lab

The overridable information is only checked by string match. If we're considering removing the spread syntax, which seems to be the primary mechanism for handling this, we'll need to find an alternative solution. One possibility could be just adding comments or exploring a different approach altogether.

I understand that there are some blocking tasks related to separate issues that need to be addressed first. I'm happy to wait until those are completed before diving into this further. So feel free to notify me when my task is ready to progress.

And sure, the alternative way requires the maintainer's review :). So any suggestion or feedback to fit yours will be welcome.

[melloware]

Looks like merge conflicts?

[karlismelderis-mckinsey]

I don't think merging arrays is an obvious, easy to grasp option

array should be overwritten with new array

[soartec-lab]

Hi, @DevSDK

We have considered handling mock overrides, but are trying to avoid nested overrides, as suggested in the PR below:

https://github.com/anymaniax/orval/pull/1338

It conflicts with the idea you suggested, so if you want to discuss it, please comment.

[DevSDK]

@soartec-lab oh wow. That change is huge.

Sorry for the late response. I was unwell though by cold...

For now, I haven't caught the context yet so please understand if I say weird below. (If it is, please ignore blow :))

We should handle nested. The change seems to handle only Partial<Response> type my thought is we should handle DeepPartial.

use case of our product was (a bit overstated)

• array
  • object - A
    • array
      • object -B
        • singleProperty - I want to test this value!

If only the root is overridable with Partial<T>, we should make whole Object A and B.

In our company, we created a fixture creator to resolve the same problem with that nested interface. For example

 function createAwesomeThingFixtures(props: DeepPartial<Type>) {
    ...
}

const fixture =  createAwesomeThingFixtures(
    [
      {
        id: '1',
        type,
        roleReference:  {
            name: "Seokho"
            // There are more than 20 properties in here! 
           // but I want to test only "name"
        }
       // and here, also have more properties
	
      },
      {
        id: '2',
        type,
        roleReference: {
            name: "Ph"
            // There are more than 20 properties in here! 
           // but I want to test only "name"
        }
      },
    ]
)

What if we write the whole 20 properties in there, I think there can be significant developer experience damage and it will be hard to change

I'll check and participate in it but now I'm a bit busy.. So... I may slow.

[soartec-lab]

@DevSDK Thank you for your response. Please take care of your body. I understand that. For objects with many properties, there may be another solution, such as making the object easier to create. At least #1338 solves the current orval having problem and I think it will be useful for many users, so I would like to merge it.

Do you have an opinion on that? If your response is slow, I'll pass it on first, but I welcome discussion, so please comment again.

[DevSDK]

@soartec-lab

Yeah. I think #1338 will cause a breaking change because the exposed function argument type changed: any -> Partial<T>

But after that, Partial<T> -> DeepPartial<T> seems safe from the breaking changes at the interface level.

So.. I agree that we can move on gradually. I'll pull that change and make supporting DeepPartial<T> that will enhance dx for nested.

[soartec-lab]

@DevSDK

Thank you for confirming.

[soartec-lab]

Hey @DevSDK , it's been a while since this PR was opened. Mocks are still being improved, so conflicts still occur. Therefore, is it okay to close this PR?

If there is an issue, it would be a good idea to open the PR again, and once the policy is organized in an issue, I think someone else can continue to deal with it. and if you would like to come back to development, I would always welcome you back.

[DevSDK]

Yep. If you prefer so.

Actually, I resolved the conflict on the local repo. (It was not as much as I thought)

So we can decide to push the merge commit with resolving the conflicts and continue gradually or simply close this PR and reopen when I'm ready. 

However, I might be slow due to the other things to do... So If we decide to close this PR, yeah, I'll do it again someday ;)

[soartec-lab]

@DevSDK

Thank you for contacting me. I have determined that the issue and approach have changed from the original purpose of creating this PR, so if it is possible to continue, could you please open the PR again? If there is no problem, I will close this PR once.

[DevSDK]

@soartec-lab Yep :) I'll do it soon. Feel free to close this