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

[Feature] reference dynamic attributes from dynamic attributes. #269

Merged
merged 5 commits into from
Mar 9, 2016

Conversation

lazybensch
Copy link
Contributor

This PR lets you reference dynamic attributes from different dynamic attributes.

Currently you are able to reference static attributes from dynamic ones like so:

// mirage/factories/user.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  age: 18,
  is_admin: function(i) {
    return this.age > 30;
  }
});

but I also want to be able to reference dynamic attributes from dynamic attributes like so:

// mirage/factories/user.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  year_of_birth() {
    return faker.random.number({ min: 1900, max: 2000 });
  },
  year_of_death() {
    return faker.random.number({ min: this.year_of_birth, max: this.year_of_birth + 90 });
  }
});

That is now possible. I also made sure it does not matter in what order you specify the properties, so it also works for:

// mirage/factories/contact.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  year_of_death() { // I reference a property that is not yet set up, but don't worry, it'll still work!
    return faker.random.number({ min: this.year_of_birth, max: this.year_of_birth + 90 });
  },
  year_of_birth() {
    return faker.random.number({ min: 1900, max: 2000 });
  }
});

What (obviously) does not work is referencing propA from propB and propB from propA. If the user tries to attempt such a mad thing, he will get a nice and helpful. Cyclic dependency in properties ["propA", "propB"]' error thrown.

I also wanted to update the docs but they don't seem to be open source, is this correct?

@lazybensch
Copy link
Contributor Author

We are using my feature branch in our production system and just found one bug when referencing multiple dynamic properties from a property. I fixed it and wrote a regression spec. I also refactored the factory code to not use exception anymore.

Other than that this feature works really nice for us. Any thoughts/concerns on the PR @samselikoff ?

@samselikoff
Copy link
Collaborator

I like it! Awesome API, I need to look at the code more (been busy lately). There's another issue related to this, originally the api was going to be Mirage.lazy, and you'd specify the keys that your attr depended on. But if this works, I think it's way better.

@lazybensch
Copy link
Contributor Author

Im glad to hear that. If you have any questions about the code don't hesitate to reach me on slack

@lazybensch
Copy link
Contributor Author

FYI, here a nice feature of that PR that i just discovered: Imagine you have a conference with 10 talks that could overlap in time but are guaranteed to start and end within the duration of the conference itself.

This is now super convenient.

server.createList('talks', 10, {

  conference_id: conference.id,

  start_date: ()=> dateBetween(conference.start_date, conference.end_date),

  end_date() { // no arrow function here to get the correct scope for `this`
     return dateBetween(this.start_date, conference.end_date);
   }

});

Just sharing this with you cause you might wanna add something alike in the docs.

@samselikoff
Copy link
Collaborator

Nice!

I haven't forgotten about this, btw.

@samselikoff
Copy link
Collaborator

Getting closer to wrapping up serializers. Factories come next.

@lazybensch
Copy link
Contributor Author

rebased this one btw

@samselikoff
Copy link
Collaborator

awesome, thank you. Factories are top priority after 0.2.x

@JakeDluhy
Copy link

I tried to pull this branch directly into my project (using "ember-cli-mirage": "git+https://github.com/lazybensch/ember-cli-mirage.git#reuse_dynamic_properties",) but when I did I got this error when trying to build.

TypeError: Cannot read property 'path' of undefined
at .../ember-cli-mirage/node_modules/ember-lodash/index.js:11:74

Any thoughts as to what that might be?

@lazybensch
Copy link
Contributor Author

@JakeDluhy, you've done everything right, thats how i import my branch into our application aswell. It seems that ember-cli can't find the lodash package in your project. maybe a rm -rf node_modules; npm cache clean; npm install will solve this problem.

If not let me know and i'll look into it again

@samselikoff
Copy link
Collaborator

As 0.2.0 is dragging out, let's go ahead and get this merged in. Would you mind rebasing + squashing your commits? After that, I'm happy to pull the trigger.

Again, thank you so much for this, awesome work on this PR.

@lazybensch
Copy link
Contributor Author

awesome
will do tomorrow eve utc+1

@mangatinanda
Copy link

@lazybensch @samselikoff When are guys planning to merge this?

@samselikoff
Copy link
Collaborator

@lazybensch if you could rebase I'm happy to get this merged in, as it's completely backwards compatible.

@samselikoff
Copy link
Collaborator

ping @lazybensch

@blimmer
Copy link
Contributor

blimmer commented Feb 22, 2016

Would love to see this, too!

@sethphillips
Copy link

i hate to leave a 'me too' but I've been watching this PR for a bit and hoping it lands

@lazybensch
Copy link
Contributor Author

@samselikoff rebased the branch again.

Sorry that it took me so long guys but, well it says 'lazy' right there in my pseudonym 😃

samselikoff added a commit that referenced this pull request Mar 9, 2016
[Feature] reference dynamic attributes from dynamic attributes.
@samselikoff samselikoff merged commit 10afcd8 into miragejs:master Mar 9, 2016
@samselikoff
Copy link
Collaborator

Haha - merged! Thanks again dude!

@MichalBryxi
Copy link
Contributor

Yes! This made my day!

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.

None yet

7 participants