Skip to content
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

Remove unnecessary 'factories' injection #21

Merged
merged 4 commits into from
Jun 29, 2020
Merged

Conversation

stevehanson
Copy link
Contributor

@stevehanson stevehanson commented Jun 8, 2020

Background

When I initially built this library, I imagined that the following situation where two factories reference each other would result in a circular import error:

// factories/user.ts
import { Factory } from 'fishery';
import { User } from '../my-types';
import postFactory from './post'; // imports post factory

const userFactory = Factory.define<User>(() => ({,
  name: 'Rosa',
  posts: postFactory.buildList(2),
}));

export default userFactory;
// factories/post.ts
import { Factory } from 'fishery';
import { Post } from '../my-types';
import userFactory from './user'; // imports user factory

const postFactory = Factory.define<Post>(({ sequence }) => ({
  title: 'My Blog Post',
  author: userFactory.build(),
}));

export default postFactory;

So, I had built a way around this by requiring users to register their factories together, which injects a factories object that can then be used by factories to indirectly reference each other:

// my-types.ts
export type Factories = {
  user: Factory<User>,
  post: Factory<Post>,
};
// factories/index.ts
const factories: Factories = register({
  user,
  post,
})

export default factories
// factories/user.ts
import { Factory } from 'fishery';
import { Factories, User } from '../my-types';

const userFactory = Factory.define<User, Factories>(({ factories }) => ({,
  name: 'Rosa',
  posts: factories.post.buildList(2),
}));

export default userFactory;

Why not necessary?

It turns out that all of this wasn't necessary and that factories can import each other without issue. Since the factories don't use each other until an object is built, the circular imports are able to resolve and then be available when build is called.

TypeScript can need some help determining the type of objects when there is a circular reference, so it can be necessary to add an explicit typing to factories when there is a circular reference:

// the extra Factory<Post> typing can be necessary with circular imports
const postFactory: Factory<Post> = Factory.define<Post>(() => ({ ...}));
export default postFactory;

The changes

  • Removes register(), as it is no longer necessary
  • Removes factories that gets passed as a parameter to factory builders
  • Removes Factory.defineUnregistered since all factories are now "unregistered". This existed as a way to define ad-hoc factories without having to call register on them. These factories did not have access to the factories object that was injected (which now doesn't exist)
  • Removes second type parameter on Factory. So Factory.define<User, Factories, UserTransientParams> is now Factory.define<User, UserTransientParams>
  • Updates sample app to demonstrate and verify that circular references are not an issue
  • Updates readme

This is obviously a breaking change. I plan to release this alongside another PR I am putting together that renames afterCreate to afterBuild and bump the major version.

@uchagani
Copy link

uchagani commented Jun 8, 2020

Thanks for your hard work on this @stevehanson . This PR really simplifies things. I've started using fishery recently and this would be a great addition.

Copy link

@emilford emilford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good to me.

@stevehanson stevehanson merged commit ff03f39 into master Jun 29, 2020
@stevehanson stevehanson deleted the remove-factories branch June 29, 2020 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants