thoughtbot / fishery

A library for setting up JavaScript objects as test data
MIT License
876 stars 36 forks source link

Factory produces data incompatible to its type constraint #106

Open lo1tuma opened 2 years ago

lo1tuma commented 2 years ago

Description

When using the factory overrides to set a property explicitly to undefined this is always allowed even when the type constraint doesn’t accept undefined as a valid value.

To Reproduce

import { Factory } from 'fishery';

interface Foo {
    bar: string;
}

const fooFactory = Factory.define<Foo>(() => {
    return { bar: 'any-value' };
});

const result = fooFactory.build({ bar: undefined }); // result has the type Foo but its value is { bar: undefined } which is not a valid Foo

Additional context

It seems like this bug has been introduced with #44

stevehanson commented 1 year ago

Thank you for reporting this issue, and apologies for the delay in getting back. This does seem to be an issue. Ensuring that factories are type-safe is more important than allowing users to un-set a property, which is what the motivation behind #44. I am leaning right now toward reverting #44 and releasing it as a major version bump.

Currently, all params passed to build are optional, and TypeScript by default allows explicitly passing undefined for properties that are typed as optional (eg. bar?: string). Ideally, TypeScript would only allow omitting the property but not passing undefined, so that fooFactory.build({ bar: undefined }) would produce a type error. This behavior is only possible with a TypeScript compiler options flag, exactOptionalPropertyTypes, which most users will not have set.

carmanchris31 commented 9 months ago

My team uses the factory to produce values for undefined properties and produce objects with no optional properties. Today I lost valuable time troubleshooting failing tests because a build function was returning an incomplete object due to this issue.

There isn't an obvious distinction between an explicitly undefined value and an implicit undefined from omission and IMO this is just too surprising and error-prone:

import {Factory} from 'fishery'

interface PointModel {
  x: number;
  y: number;
}

const pointFactory = Factory.define<PointModel>(() => {
  return {
    x: 0,
    y: 0
  }
})

const data: Partial<PointModel> = {x: 5}

const a = pointFactory.build(data)
console.log(a) // {x: 5, y: 0}
const b = pointFactory.build({ x: data.x, y: data.y })
console.log(b) // {x: 5, y: undefined}

https://playcode.io/1777612

I would be in favor of reverting with a major version bump.