Remove existing value with undefined

[rikardaxelsson]

Sometimes you want to remove existing data, but it seems like null and undefined are handled differently. The code below returns { firstname: 'First', lastname: null }, but i would expect { firstname: undefined, lastname: null }.

import { Factory } from "fishery";

type MyType = {
  firstname?: string;
  lastname: string | null;
};

export const myFactory = Factory.define<MyType>(() => ({
  firstname: "First",
  lastname: "Last",
}));

console.log(
  myFactory.build({
    firstname: undefined,
    lastname: null,
  })
);

[sPaCeMoNk3yIam]

When you say removing I thought you'd expect { lastname: null } as a result of build (like in JSON).

[stevehanson]

Thanks for reporting this, @rikardaxelsson. I've been on extended leave after having a child but am hoping to look at this one more soon. If you wanted to experiment with this, I'd welcome your contributions. I'm guessing the place to start would be to try to override the lodash merge behavior by modifying our mergeCustomizer here: https://github.com/thoughtbot/fishery/blob/main/lib/merge.ts#L4. And here's the lodash documentation. Thanks again!

[siquel]

@stevehanson experimented little with that.
It seems that lodash merge behavior handles undefined here.

If customizer returns undefined, merging is handled by the method instead

As lodash docs say, if mergeCustomizer returns undefined, the merge is then handled by lodash, and it will then default to srcVal, which is the previous value there used to be. To overcome this, we can return null instead, as it's not handled same way as undefined, but this feels kinda hacky.

const mergeCustomizer = (_object: any, srcVal: any) => {
  if (Array.isArray(srcVal)) {
    return srcVal;
  } else if (srcVal === undefined) {
    return null;
  }
  return undefined;
};

const result = mergeWith(
  {}, 
  { foo: 'bar', fizz: 'buzz', buzz: 'lightyear' },
  { foo: undefined, fizz: null, a: 5 },
  mergeCustomizer
) // {foo: null, fizz: null, buzz: "lightyear", a: 5}

[stevehanson]

@siquel thanks for looking into that. I'm not sure when I'll have more time to look into this. In the meantime, I welcome any community thoughts or contributions. It seems like Lodash's merge just might not support this behavior, in which case we might need to find a different merge implementation.

To get around this right now, I recommend using an afterBuild to set a value back to undefined:

const userWithoutNameFactory = userFactory.afterBuild(user => { user.name = undefined; return user});
const user = userWithoutNameFactory.build();
user.name // undefined

To add some syntax sugar to that, you could write your factory as a class and define methods on the factory:

class UserFactory extends Factory<User> {
  withoutName() {
    return this.afterBuild(user => { user.name = undefined; return user});
  }

  withoutPosts() {
    return this.afterBuild(user => { user.posts = undefined; return user });
  }
}

const userFactory = UserFactory.define(() => ({ ... }))

const user = userFactory.withoutName().withoutPosts().build();
user.name // undefined
user.posts // undefined

[rakeshpetit]

@stevehanson - I might have successfully found a solution to this problem without doing the merge in an afterBuild phase. Please let me know if the fix would cause problems. I will be happy to work on a failing test.

[stevehanson]

This was closed by #62 and has been released in version 1.4.0.

[JonathanGuo]

Appreciate the issue and I understand the solution. But I have to say the PR #62 just introduced a breaking change.

In my case, I have a very complex data model I need to build. I pass a transient parameter to FactoryA to conditionally generate FactoryB with some default values and other nesting models. The old merge logic of the undefined property is handy when I want to fall back to the default value.

e.g.

// FactoryA:

{
  relationA: RelationAFactory.build({
      propertyA: transientParams.propertyA,
      propertyB: transientParams.propertyB,
 }),
}

// Factory B:

{
  propertyA: params.propertyA || defaultValueA,
  propertyB: isNil(params.propertyB) ? defaultValueB :  params.propertyB,
}

So my test cases know the mocked data always has values for the properties.

But with the merge including the undefined value, the default values are replaced again after returning from the factory.
Even put the fallback logic into afterBuild the undefined value will still be merged afterwards.

The workaround that I have to do now is adding extra logic in my FactoryA to filter out the undefined properties. No biggie, but I think it's worth noting down the breaking changes.

(🚨 Please let me know if I am building my factory in a wrong way)

[luhmann]

I have the same problem has @JonathanGuo and would agree that it introduced a breaking change.

I also used the original behaviour of undefined in a way to reset a specific value to the default of the factory. Here is a small, contrived example of how it used to beneficial to me:

// you have a cms-like app where you have existing entities and new drafts which start out as copies 
// of the last published version

const artistBuilder = Factory.define<Artist>(() => ({
  __typename: "Artist",
  name: "Bono"
}))

const artistDraftBuilder = Factory.define<ArtistDraft>(() => ({
  __typename: "ArtistDraft",
  name: "Michael"
}))
 
const publishedVersion = artistBuilder.build({ name: "Gaga" })

// `publishedVersion` contains an incorrect `__typename` which I want to reset to `ArtistDraft` from the original factory, I can of course set it explicitly but the actual code does this is in a generalized fashion for lots of different data-types where it was beneficial that I did not need to know the concrete type 
const draft = artistDraftBuilder.build({ ...publishedVersion, __typename: undefined })

I agree that it is no big deal and the intended change makes sense to me. It is probably more to peoples expectations as Object.assign or the spread-syntax work this way. But you might want to make it clearer in the Release Notes that this can break your code which I wouldn't expect from a minor semver-change.