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

Failing test to show that afterMake is run before the model's onLoad hook #259

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lorcan
Copy link
Contributor

@lorcan lorcan commented Nov 16, 2016

I came across this behaviour that seems a little unexpected, I wonder if it's a bug or maybe something I'm doing wrong... certainly I'm doing something complex and likely very edge-case. This PR provides failing tests to demonstrate the behaviour I'm seeing.

I have a model that sets some derived attributes in a didLoad model hook. This works as expected when loading the model normally.

However, when I build a model with a factory that has an afterMake function I might expect that it the afterMake function is called so that my model is fully populated before the model's own didLoad hook is triggered. This isn't the case however because of the sequencing here:

    const model = Ember.run(()=> this.store.push(data)); // <-- didLoad is triggered at this point

    let definition = lookupDefinitionForFixtureName(args.name);
    if (definition.hasAfterMake()) {
      definition.applyAfterMake(model, args.opts); // <-- and then afterMake is called
    }
    return model;

I was trying to think about how this might be reordered and it's not obvious to me whether it could, or even would be worth the complexity that would introduce - you kind-of need a model for the afterMake function.

Another solution might be to trigger didLoad right before returning model, but it might not be ideal to be firing two didLoads:

    const model = Ember.run(()=> this.store.push(data)); // <-- didLoad is triggered at this point

    let definition = lookupDefinitionForFixtureName(args.name);
    if (definition.hasAfterMake()) {
      definition.applyAfterMake(model, args.opts);
    }
    model.trigger('didLoad'); // <-- now we explicitly trigger didLoad again
    return model;

Any thoughts? I'm happy to flesh this PR out with a fix if you could give me some direction :-)

@danielspaniel
Copy link
Collaborator

This is an edge case, but an interesting one none the less.
I guess it depends on what you want afterMake to do.
I would think afterMake would run before didLoad also, but I have not thought about it for long.

Are you using afterMake to just setup data with transient attributes?
Can you show me how you are setting up your factory and using it.

  // don't use afterMake at all but still use transient attributes that make your fixture 

  let data = this.fixtureBuilder(modelName).convertForMake(modelName, fixture);   
  const model = Ember.run(()=> this.store.push(data)); // <-- didLoad is triggered at this point
  return model;

Here is how I use transient attributes to build fixture:

https://github.com/danielspaniel/ember-data-factory-guy#tip-3-using-random-attributes-to-build-fixture

Maybe don't need afterMake?
Maybe get rid of it if it does not work as people expect?

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.

2 participants