AMD/Require and use strict #21

davetheninja · 2014-09-04T06:15:09Z

Hey,
I really am quite interested in using this library for dev/testing however in its current state I am unable to use it.

Issues:

I use 'use strict' within my build process which rejects undeclared (var) variables. var is not used when defining classes within this library.
Unless I am missing something obvious, there is no support for AMD/Require JS

Question:

I am more than willing to fork, change and submit a patch to resolve the above - is this something you would be willing to accept into your library?

If you are totally against strict and AMD then there is no point me spending time forking and patching.

Thanks

Davie

OpakAlex · 2014-09-04T06:16:31Z

Hi! It's good idea, can you add this, and create PR?

danielspaniel · 2014-09-04T06:20:38Z

@davetheninja .. I would be happy to merge your patch with these new additions, so go for it.

davetheninja · 2014-09-04T06:22:30Z

Most excellent. I will get on the case. One other question, i noticed in the gruntfile there is grunt-contrib-coffee which is not used within the build proccess.

Is this something you were looking to do, migrate to coffeescript?

danielspaniel · 2014-09-04T06:32:19Z

I was going to write the tests in coffeescript, but not the core code. I was having some trouble with async tests in coffeescript so I abandoned that idea and kept with the js, but I could revisit that one day.

davetheninja · 2014-09-04T12:56:14Z

Can you confirm if the master tests pass on your machine? Trying to run them and they are failing on

Error: Assertion Failed: The content property of DS.PromiseArray should be set before modifying it

Wondering if its a bug or my machine. Thanks

davetheninja · 2014-09-04T12:59:31Z

Just spotted the travis file. Checked on TravisCI and can confirm it master thats failing not my local.

Ill see if i can fix that while im at it

danielspaniel · 2014-09-04T13:08:01Z

That was a strange one .. because my tests pass .. but when I see that error on Travis, so it's nice that you can reproduce that .. because I could not. If you can fix .. even better.

davetheninja · 2014-09-04T13:12:05Z

What version of ember/ember data are you running locally.

danielspaniel · 2014-09-04T13:16:27Z

1.0.0-beta.8.2a68c63a

davetheninja · 2014-09-04T13:21:01Z

Test in question is #createRecord adds belongsTo association to records it hasMany of within fixture adapter factory test

danielspaniel · 2014-09-04T14:50:24Z

Good to know .. but I am not getting that error .. what versions of ember / ember-data are you running?

davetheninja · 2014-09-04T14:53:32Z

i have tried a few different, from a bower install (using whats specified in the bower.json) and have upped the versions incrementally to beta.9 - no dice

currently stepping through code in ember to see why its blowing up

danielspaniel · 2014-09-04T14:55:55Z

ok .. I wish I could reproduce it myself .. so thanks for trying to track it down.

davetheninja · 2014-09-04T14:56:36Z

sure. what OS are you running? (not sure it would make a difference though)

danielspaniel · 2014-09-04T14:58:45Z

mavericks .. and what versions of ember and ember data get you this failure .. ( all of them? it sounds like )

davetheninja · 2014-09-04T15:04:02Z

from ember 1.6 -> 1.8 beta 1 and ember-data 1.0.0-beta.7 -> 1.0.0-beta.9

davetheninja · 2014-09-04T15:10:47Z

Its something to do with has many and async (RSVP call within the user find promise) . Ill have another look at it tomorrow

davetheninja · 2014-09-04T15:15:24Z

Right. So I went back to Travis to check the logs.....

a1181df

That was the commit that caused the build to fail.

I hate computers.

danielspaniel · 2014-09-04T15:18:00Z

geez .. that commit does not even do anything in particular .. which is why I just threw my hands up and ignored the travis test failure, since it seemed bogus. But now that you are actually reproducing it and have the bug live and fresh on your machine .. well .. yeah .. good point about computers.

davetheninja · 2014-09-04T15:20:42Z

Right. Just to err on the side of caution, I checked out the GREEN tag. Still doesnt work on my machine.

Could you try a clean clone of the repo, bower install, npm install grunt test

To see if it happens on a clean build on your machine

davetheninja · 2014-09-04T15:24:32Z

AH HA!!! I figured it out.

davetheninja · 2014-09-04T15:27:23Z

Last passing Travis Build.....

bower install ember-data#1.0.0-beta.8
bower install jquery#1.10.2
bower install ember#1.6.1

First Failing

bower install ember-data#1.0.0-beta.9
bower install jquery#1.10.2
bower install ember#1.6.1

I forced install 1.6.1 and 1.0.0-beta.8 locally and now the tests pass.

davetheninja · 2014-09-04T15:28:29Z

So to get a green build on travis, put a hard dependency on 1.6.1 and beta 8 for the time being.

davetheninja · 2014-09-04T15:28:55Z

Victory is sweet. Time for beer.

danielspaniel · 2014-09-04T15:30:34Z

Hooray .. for you .. and for me .. I got the test to fail with fresh install ( looks like newer ember release kills that test .. but I will try and fix also ) ..

davetheninja · 2014-09-04T15:33:33Z

Sure, I need a green build before I start committing changes - need a solid platform if you know what I mean.

Expect Pull requests tomorrow instead of tonight :-)

danielspaniel · 2014-09-04T15:35:15Z

no problem .. will be nice to get this sorted out

danielspaniel · 2014-09-04T17:12:49Z

I have been looking at this and the code in DS.FixtureAdapter.createRecord .. ( which is where the problem is ) and to me .. its not worth fixing this .. since I am thinking it is best to drop support for the fixture adapter anyway ..
the purpose of this project is that you don't have to use the fixture adapter, so why am I supporting it ( out of legacy is the answer .. but I barely bother with keeping fixture adapter happy ) ..
so if you start to feel like your going down a deep well and getting nowhere ..

davetheninja · 2014-09-05T00:02:57Z

Agree.

—
Sent from Mailbox

On Fri, Sep 5, 2014 at 1:12 AM, Daniel Sudol [email protected]
wrote:

I have been looking at this and the code in DS.FixtureAdapter.createRecord .. ( which is where the problem is ) and to me .. its not worth fixing this .. since I am thinking it is best to drop support for the fixture adapter anyway ..
the purpose of this project is that you don't have to use the fixture adapter, so why am I supporting it ( out of legacy is the answer .. but I barely bother with keeping fixture adapter happy ) ..

so if you start to feel like your going down a deep well and getting nowhere ..

Reply to this email directly or view it on GitHub:
#21 (comment)

danielspaniel · 2014-09-05T05:10:54Z

I mentioned the test failure to @KnownSubset .. and asked him if he could try and fix it, since he has more interest in making that code work. If he can't fix it .. I will comment out the test so the build passes and wait for inspiration to fix it myself.

davetheninja · 2014-09-05T11:53:42Z

any reason why the has_many.js include in the qunit index.html is not being referenced from the dist package?

OpakAlex · 2014-09-05T12:02:23Z

i think has_many.js is outdated. In latest ember-data it's not works. I have commit to up it on latest version, but we need change tests for FixtureAdapter. Build belongs_to is broken.

davetheninja · 2014-09-05T12:02:56Z

p.s I have a basic setup working now where RequireJS is now supported.

I am going to refactor the test structure so that I can run all tests from Global scope and through Require.

davetheninja · 2014-09-05T12:03:07Z

Ok No problem

OpakAlex · 2014-09-05T12:04:32Z

@davetheninja You can see my last 3 commits https://github.com/OpakAlex/ember-data-factory-guy/commits/master

OpakAlex · 2014-09-05T12:05:06Z

Do you use broccoli for you build?

davetheninja · 2014-09-05T12:06:11Z

Personally I do use broccoli yes, I have no problem whatsoever using Grunt though given it is already integrated.

Would Broccoli be something you would want to use?

OpakAlex · 2014-09-05T12:07:43Z

yes, i want new version with Broccoli. It will be very nice, and we can add some console commands for ember-cli. #17 Check this one.

davetheninja · 2014-09-05T12:15:09Z

Yeh thats quite sweet indeed :-)

To be honest with you guys, looking through the googlz this is the first project that has come close to what I am looking for in regards to streamlined testing within ember.

I am completely against use of fixtures as from experience (rails etc) they get out of control very quickly.

Factories on the other hand are much easier to maintain and override on a test per test basis.

Even more honestly (or stupidly) I have not even got this project to run on my app yet (amd dependency) hence I am working to add the requirejs goodies - just to test the app :-)

Once its added and I see if I can use this project, going forward I will be more than happy to invest time contributing.

As I said, from the features you currently support it looks almost perfect for my use case.

Bravo

davetheninja · 2014-09-05T12:23:04Z

One other thing I would recommend is using broc + es6 concat or similar and setting up the dependencies within your source files:

Example:

import Sequence from './sequence'

var FactoryGuy = {

  someMethodThatUsesSequence: function() {
    return Sequence.generate();
  }

}

export default FactoryGuy

The above is completely trivial example but that pattern has helped me so much (even more so when combined with CoffeeScript) when architecting my JS apps and libs.

OpakAlex · 2014-09-05T12:38:50Z

If you need help to testing your Ember app you can ping me.

davetheninja · 2014-09-05T13:03:40Z

Thanks

KnownSubset · 2014-09-05T14:35:39Z

@davetheninja I think everyone is against hard coded fixtures, but using a library/factories that populate the list of fixtures is an entirely different thing. I do think that the library should become adapter agnostic so that we rely only upon the store otherwise ember-data-factory-guy's adoption will always be hindered if a new adapter comes out.

davetheninja · 2014-09-05T14:37:27Z

Exactly. That's what I meant.

Factory -> attributes
factory -> store

—
Sent from Mailbox

On Fri, Sep 5, 2014 at 10:35 PM, Nathan [email protected] wrote:

@davetheninja I think everyone is against hard coded fixtures, but using a library/factories that populate the list of fixtures is an entirely different thing. I do think that the library should become adapter agnostic so that we rely only upon the store otherwise ember-data-factory-guy's adoption will always be hindered if a new adapter comes out.

Reply to this email directly or view it on GitHub:
#21 (comment)

danielspaniel · 2014-09-06T11:27:45Z

@davetheninja .. I am not convinced on the need for switching to brocolli, so don't include that in this pull request ( lets save for another time ) And @OpakAlex, if you want this switch from grunt, try and implement it yourself first.

davetheninja · 2014-09-06T16:23:44Z

Sure. As I already said I have no intention to do anything other than requirejs initially.

—
Sent from Mailbox

On Sat, Sep 6, 2014 at 7:27 PM, Daniel Sudol [email protected]
wrote:

@davetheninja .. I am not convinced on the need for switching to brocolli, so don't include that in this pull request ( lets save for another time ) And @OpakAlex, if you want this switch from grunt, try and implement it yourself first.

Reply to this email directly or view it on GitHub:
#21 (comment)

danielspaniel · 2014-09-06T20:55:50Z

Got it .. and I am looking forward to seeing that in action. :)

danielspaniel · 2014-11-09T11:36:58Z

Hi @davetheninja .. how is it going with the amd conversion? I am guessing it got stalled .. but hoping you can finish it one of these days :)

danielspaniel · 2014-12-05T08:01:45Z

@indirect made this happen so I am closing this one.

davetheninja · 2015-07-20T14:57:39Z

Apologies for being AWOL, family and work commitments. I am actually getting time to do some of my own project coding again.

The new factory-guy is excellent btw. Well done

danielspaniel · 2015-07-20T14:59:41Z

Thanks @davetheninja .. mucho appreciado and glad to see you back :)

davetheninja · 2015-07-20T15:55:10Z

😌

danielspaniel closed this as completed Dec 5, 2014

AMD/Require and use strict #21

AMD/Require and use strict #21

Comments

davetheninja commented Sep 4, 2014

OpakAlex commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

Last passing Travis Build.....

First Failing

davetheninja commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

danielspaniel commented Sep 4, 2014

davetheninja commented Sep 5, 2014

so if you start to feel like your going down a deep well and getting nowhere ..

danielspaniel commented Sep 5, 2014

davetheninja commented Sep 5, 2014

OpakAlex commented Sep 5, 2014

davetheninja commented Sep 5, 2014

davetheninja commented Sep 5, 2014

OpakAlex commented Sep 5, 2014

OpakAlex commented Sep 5, 2014

davetheninja commented Sep 5, 2014

OpakAlex commented Sep 5, 2014

davetheninja commented Sep 5, 2014

davetheninja commented Sep 5, 2014

OpakAlex commented Sep 5, 2014

davetheninja commented Sep 5, 2014

KnownSubset commented Sep 5, 2014

davetheninja commented Sep 5, 2014

danielspaniel commented Sep 6, 2014

davetheninja commented Sep 6, 2014

@davetheninja .. I am not convinced on the need for switching to brocolli, so don't include that in this pull request ( lets save for another time ) And @OpakAlex, if you want this switch from grunt, try and implement it yourself first.

danielspaniel commented Sep 6, 2014

danielspaniel commented Nov 9, 2014

danielspaniel commented Dec 5, 2014

davetheninja commented Jul 20, 2015

danielspaniel commented Jul 20, 2015

davetheninja commented Jul 20, 2015