ember-data-factory-guy
ember-data-factory-guy copied to clipboard
adopted-ember-addons
Default arguments if `fails`
Not sure if that's a problem of CoffeeScript which we use, or just something general, but when we use something like:
mockUpdate(@user).match(first_name: '').fails(status: 422, response: {errors: {first_name: ["can't be blank"]}})
It takes convertErrors as false. So I guess it's some issue with the args notation here: https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mocks/mock-request.js#L43 Might worth rewriting it good 'ol way to support CoffeeScript users.
Though it's weird that it doesn't work for me, cause CoffeeScript does support destructuring assignment and after compilation it must be just as passing plain object into a function with such arguments. Maybe that's babel that takes that stuff off on transpiling?
hm .. that is kind of a bummer because me like that es6 notation alot. not sure why would make a difference if using coffescript .. but I will write something in coffeescript and see if I get same problem and then undo that fancy notation if I have too :( will make me sad though. Can you not use coffeescript ( ?? ) .. Did I just say that? I took all my tests in this one project and started converting away from coffee a few months ago, since es6 just about makes it not necessary anyway .. and I feel like it is the right way to go anyway .. so .. just throwing that out there. but I agree it should not break for coffeescript users
Well, you're the owner of this lib, so that's your call :) I can explicitly set properties in such cases, which actually I did when figured out the problem, but other users can run into this issue as well, that's my main concern. So it can be told somewhere in documentation, as an easiest option.
I like that shiny ES6 features too, and if they were there when I started using Coffee, may be I wouldn't :) And to be honest, that should work just fine, as exactly the same notation of destructuring arguments assignment was in CS before, so maybe it's something tiny that could be fixed easily.
I agree though about everything. It should work for coffeescript users .. I am sure that is a bug somewhere in the chain of happenings ( babel to coffee compilers ) .. so, yes I will fix it ..
We can help with that if you want. Also I have couple of ideas, for one of which my mate is going to make a PR by tomorrow.
Now your talking bro .. I'll take the help .. I am a bit overstretched these days .. and this kind of fix is pretty swift and painless
Sorry, been pretty busy. Will try to get to this soon!
Thanks for #231 btw! 👍 👍 👍
I think I need your comment on this one: https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mocks/mock-update-request.js#L54
Who said that update errors don't need conversion? :-\ Now I'm puzzled too... I don't understand the reason of overriding this method.
I think I ran the tests and I needed to have that override ( which puzzled me ). But if things now work without it .. be my guess and blow that away.
So that means there's somewhere in the tests wrong expectation? Anyways, lemme check that.
Oh, I see... So to fix that we either have to pass convertErrors: false in tests, or expect errors in appropriate format:
https://github.com/danielspaniel/ember-data-factory-guy/blob/master/tests/unit/shared-factory-guy-test-helper-tests.js#L1732-L1751
But there's another problem, I thought that convertErrors option is Adapter specific, but when true it always returns json-api style errors. Should we delegate conversion to adaptors instead?
The errors end up json-api style in the end anyway ( as I recall ) If that works ( to delegate to adapter ) .. then great, so I don't mind either way.
If that's the case, why do we need option at all? But I'm not sure about that. As I recall, there was a difference... I have to run right now, but will research and be back soon. As for PR, I think it's safe to merge right now. That error treating thing is not really related.
So you were right, that internally the errors become the same json-api style, but what is generated by back-end is different: Here's what RESTAdapter expects: http://emberjs.com/api/data/classes/DS.RESTAdapter.html#toc_errors And JSONAPI expects the one described by the specs, so I assume it should be generated in different manner depending on the adapter used.
Also, I found there are convenience methods provided by ember-data, that could be used instead of own implementation here: https://github.com/AutoCloud/ember-data-factory-guy/blob/master/addon/builder/fixture-builder.js#L94-L105
And methods are here: https://github.com/emberjs/data/blob/fe076470b77e2d6abd614bc8710c37fb31b64e3b/addon/index.js#L121-L122 https://github.com/emberjs/data/blob/master/addon/adapters/errors.js#L168-L223
You want to take a shot at changing from my impementation to the ember-data one?
Yeah, I can do that, but it's again just a part of the story. You need to decide what to do with this whole errors handling thing. My understanding is that errors should be provided in general way and then converted per adaptor, so that option convertErrors becomes obsolete. Also it seems redundant to write: response: {errors: {....}, it could be just errors: {....}, as we don't expect anything else in fails() handler, or do we?
Oh, and json-api declares format for batching the errors, like shown here: http://jsonapi.org/examples/#error-objects-multiple-errors I've never used it this way and not even sure, what could be the purpose, but shall we support it too?
your right .. i think response could be changed to just errors and I don't love the convertErrors thing much at all, so if we remove that I would be pretty happy. I put it there because of this: https://github.com/danielspaniel/ember-data-factory-guy/issues/215 but the idea there was for me to stop converting error hash automatically so she could do json-api formatted. So, if we allow adapter to "convert" the error response, as long as it basically keeps anything formatted properly for that adapter the way it is .. and converts others to the proper format, then we are good to get rid of it.
thanks for the attention to detail and yes, if we can support batch errors that would be nice
Actually, I think we could do something neater... I just quickly read that #215 issue and figured, why don't we leave response as something RAW that could be provided by the user. For example, using meta in error response seems weird for me, but if someone needs that, they can supply response and it'll be passed as is. But if you supply errors parameter, it'll be converted respectively to adapter.
The same for status code, usually it's just 422, so if we talk about field errors, that could be set automatically. Anyways, I'll think a bit about it and be back with PR.
true about 422, and that sounded ok about the errors/meta thing. I think your on the right track so just go nuts and see what happens.
Yeah, was buried by more urgent tasks. If open issue bothers you, let's close it for now and I'll return to it when I have a chance.
I am not really bothered by open issue. Just be nice to get it out of the way, since things are always changing and it is harder to finish it ( without merge issues ) when a PR sits too long. If you want me to help out, let me know. I have time to work on it.
We just ran into an issue with mockFindRecord and returns(model: ...) after version update, so we'll be investigating after thanksgiving and address this thing too.
Ok .. sure thing, and as I said, I am happy to pitch in and help finish.