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

Factory traits #852

Closed
samselikoff opened this issue Aug 30, 2016 · 7 comments
Closed

Factory traits #852

samselikoff opened this issue Aug 30, 2016 · 7 comments

Comments

@samselikoff
Copy link
Collaborator

Traits let you have different "versions" of a factory:

// factories/post.js
import { Factory, trait } from 'ember-cli-mirage';

export default Factory.extend({

  title: 'Lorem ipsum',

  published: trait({
    isPublished: true,
    publishedAt: '2010-01-01 10:00:00'
  }),

  withComments: trait({
    afterCreate(post, server) {
      server.createList('comment', 3, { post });
    }
  })

});

Invoke traits during server.create, by passing in a variable-length list of trait names:

let defaultPost = server.create('post');
let publishedPost = server.create('post', 'published');
let postWithComments = server.create('post', 'withComments');
let publishedPostWithComments = server.create('post', 'published', 'withComments');

You can still pass in overrides, by passing in an object as the last argument:

let publishedPost = server.create('post', 'published', { title: 'Rails is omakase' });
@Azdaroth
Copy link
Collaborator

Azdaroth commented Sep 2, 2016

I will give it a try this weekend if nobody has started working on traits yet, definitely a super useful feature.

@samselikoff
Copy link
Collaborator Author

Would be amazing!

Some folks had asked about a chaining API instead, but I can't think of one I like.

@Leooo
Copy link
Contributor

Leooo commented Sep 4, 2016

@Azdaroth @samselikoff yes, I think #849 is better.

You want to write real-life-like scenario for your seed-data, and this has to be flexible to encompass multiple cases / trees: so server.create('customer').hasOne('premise').hasNo('energyAccount').hasOne('serviceAccount').hasOne('appointment', attrsForAppointment) really seems the natural way to go

  • Factories are responsible for defining and creating seed data for attributes
  • Models are responsible for defining and creating relationships

What's wrong with that?

@Azdaroth
Copy link
Collaborator

Azdaroth commented Sep 4, 2016

Maybe I got too much used to using factory_girl, but still I would say traits are more natural to work with and the API is less complex, they work just as an extra extensions.

At the end of the day, I think the difference between working with traits and chaining API would be server.create('article', 'withComments', 'published') vs server.create('article').withComments().published(). Not really sure if it's a good idea to have this setup split between the factories and the models, it forces you to check both factory and the model if you want to understand what is actually handled where. For the associations I certainly see some benefits like ability to pass overrides instead of doing it in afterCreate hook, but something like published which doesn't handle associations, but only adds some extra attributes, definitely looks like some trait that should belong to factory.

@Leooo
Copy link
Contributor

Leooo commented Sep 4, 2016

@Azdaroth I would say nothing stops us from having traits for simple scenarios, and model hooks for bigger trees (not many hooks, only hasOne, hasMulti, hasNone and probably an init hook). Let's see what @samselikoff thinks, I can always use my fork of ember-cli-mirage but I'd rather see this in the master branch as it really feels like a must-have to me.

@samselikoff
Copy link
Collaborator Author

samselikoff commented Sep 4, 2016

I spent some time trying to think of a chained API so we don't just cargo-cult factory_girl, but I couldn't think of one that doesn't introduce breaking changes. The problem with

server.create('post').hasMany('comments')

is that server.create('post') currently returns a post model. So, the above API would be a breaking change, as server.create('post') would need to return something like a factory instance, then we'd need to unwrap it to return the actual model:

server.create('post')
  .hasMany('comments')
  .hasOne('author')
  .value();

I then thought of how we could add traits as pure functions, the way ExMachina works, something like

// factories/post.js
export default Factory.extend({
  title() {
    return faker.lorem.sentence();
  },

  traits: {
    published(post, server) {
      post.update('isPublished', true);
    },

    withComments(post, server) {
      server.createList('comment', 3, { post });
    }
  }
});

but alas JS has no types or piping, and this sucks:

published(withComments(server.create('post')))

(not to mention those traits would need to be imported). Tried to think of a lot of other things but ultimately couldn't think of anything else without breaking API. We could add (now or eventually) another object that lets you chain:

factory.create('post').published().withComments()

but, yeah. Not backwards-compat.

One last point, just adding the .hasMany and .hasOne to the server.create layer, I'm not 100% I like, because traits are supposed to encourage developers to encapsulate logic within their factories, and keep irrelevant object-graph knowledge outside of their tests. So from that perspective, I like keeping server.create underpowered, to encourage devs to write domain-meaningful traits.

@samselikoff
Copy link
Collaborator Author

235945d closes

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

No branches or pull requests

3 participants