Moves out relation responsibilities from Mapper class #405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
=========================================
- Coverage 95.97% 95.87% -0.1%
=========================================
Files 21 21
Lines 2508 2520 +12
=========================================
+ Hits 2407 2416 +9
- Misses 101 104 +3
Continue to review full report at Codecov.
|
|
@jmdobry simplified tests a bit and found a bug, fixed that =) Plan to continue with tests for |
|
Also it would be useful if you could describe all relations between classes, and stuff like why we subscribe to all collection/record events, etc. I mean describe somewhere in doc/wiki |
|
Sorry it took me so long to get back to you on this. Here is where I'm describing the relations, I hope it helps: http://www.js-data.io/v3.0/docs/relations What's the status of adding more tests for this? (Plus needs to be updated with the latest |
|
There are 2 tests in create.js and all tests in createMany.js files They are extremely huge. Hard to understand sometime the logic behind the code |
src/Collection.js
Outdated
| @@ -264,7 +264,7 @@ export default Component.extend({ | |||
| // Here, the currently visted record does not correspond to any record | |||
| // in the collection, so (optionally) instantiate this record and insert | |||
| // it into the collection | |||
| record = this.mapper ? this.mapper.createRecord(record, opts) : record | |||
| record = this.mapper && !this.mapper.is(record) ? this.mapper.createRecord(record, opts) : record | |||
There was a problem hiding this comment.
This would be a breaking change, as createRecord does more than just wrap the props in the Mapper's Record class. It also checks the record's relations and makes sure have been wrapped too. createRecord doesn't double wrap records, so you don't need the !this.mapper.is(record) check.
There was a problem hiding this comment.
According to my debugging record is always here an instance of mapper and if not then it should be converted through createRecord. JSData supports only 3 ways of model creation: createRecord, createInstance, wrap (all goes through createRecord). All others should be unacceptable. If people want to instantiate model using new it shouldn't work properly and this is what should be noted in docs.
Please correct me if I'm wrong, so then I will change back this
There was a problem hiding this comment.
You are correct, but I still think there's a use case you're missing by adding that additional check to the conditional expression. e.g.
Before your change:
let user = store.createRecord('user', { id: 1 });
user.profile = { id: 1, user_id: 1 };
store.is('profile', user.profile); // false
user = store.add('user', user);
store.is('profile', user.profile); // trueafter your change:
let user = store.createRecord('user', { id: 1 });
user.profile = { id: 1, user_id: 1 };
store.is('profile', user.profile); // false
user = store.add('user', user);
store.is('profile', user.profile); // false, uh oh!There was a problem hiding this comment.
I see that make sense. Will revert
| } | ||
| return undefined | ||
|
|
||
| return errors.some(Boolean) ? errors : undefined |
There was a problem hiding this comment.
Does Babel transpile uses of Array.prototype.some into ES5-compatible code? If not then we can't use it.
There was a problem hiding this comment.
that's pity, the same story as in tests doesn't work here =) So, probably I need to change. But again this is only about IE8, does it make sense to care about it? https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/some
http://caniuse.com/usage-table
Let me know if you want to revert this back
There was a problem hiding this comment.
Need to change as JSData v3 declares than it requires no higher than ES5 support in order to work.
There was a problem hiding this comment.
Sorry for being a pain, but every/some methods are part of ES5.
http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.17
There was a problem hiding this comment.
But anyway this is up to you. If you say don't use some/every because of IE8 then I will change this
There was a problem hiding this comment.
Ah you are correct, I don't know why I was thinking every and some were introduced in the ES2015 spec.
There was a problem hiding this comment.
By the way, I've just found out that Array.isArray is also a part of ES5 spec :) http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.3.2
test/unit/mapper/create.test.js
Outdated
| assert.equal(user, undefined, 'user was not created') | ||
| assert.equal(props[User.idAttribute], undefined, 'props does NOT have an id') | ||
| }) | ||
| it('should disallow extra props', async function () { |
There was a problem hiding this comment.
Did this test just get totally deleted? It needs to stay.
There was a problem hiding this comment.
Yes deleted this test because the only thing which we need to test here is that Schema#validate is called and the rest of schema related checks should be done in schema tests. Otherwise you need to duplicate not only disallowed props but all kind of checks (required, minlength, min/max, propsPatterns, etc). All that tests were fixed in #417.
Responsibility of this tests is just to test creation of resources, not their validation. Plus there is a small set of tests which checks what happens if record is invalid, you can find them by searching describe('when invalid object is passed'
So, does it make sense now?
There was a problem hiding this comment.
Well, maybe this test needs to get moved into the integration test suite, as it's testing a more complex code path than what the unit tests cover.
|
Needs to be updated with master. |
The only thing left is to move |
|
I think that refactoring of It's even now pretty big Update: |
|
@jmdobry happy you are back :) For some reason > nyc --require babel-core/register --require babel-polyfill --cache mocha --recursive -t 20000 -R dot && nyc report --reporter=html
events.js:160
throw er; // Unhandled 'error' event
^
Error: spawn mocha ENOENT
at exports._errnoException (util.js:1026:11)
at Process.ChildProcess._handle.onexit (internal/child_process.js:193:32)
at onErrorNT (internal/child_process.js:359:16)
at _combinedTickCallback (internal/process/next_tick.js:74:11)
at process._tickCallback (internal/process/next_tick.js:98:9)
at Module.runMain (module.js:606:11)
at run (bootstrap_node.js:394:7)
at startup (bootstrap_node.js:149:9)
at bootstrap_node.js:509:3Could you please take a look? |
|
@jmdobry looks like everything is fine now. Can we merge this? So, I will be able to proceed on the next steps |
|
@jmdobry any news? |
It took a while to understand and fix the code of Mapper.js, so I also plan to add few unit tests related to create/createMany methods and send this a bit earlier just to make you to be familiar with changes. Probably they are not simple to understand, so I will glad to answer any questions you have