T/132: Two–way data binding between Collection instances #140
Conversation
src/collection.js
Outdated
| * @protected | ||
| * @member {Map} | ||
| */ | ||
| this._bindToInternalToExternalMap = new Map(); |
There was a problem hiding this comment.
Can't it be a WeaMap? Does not look like a real collection more like mapping and WeaMap wound be safer for mapping because it will be cleared even if one forget to call clear. I see you iterate over this map only in tests what seems to be too deeply testing, checking internal, what is a bad smell. Can't test check only the content of collections without checking internal?
There was a problem hiding this comment.
Also, it's not clear what these variable are used for. What is external items? What is internal? A longer description would be useful since there variable seems to be essential for the binding.
There was a problem hiding this comment.
I'll check this out. What you suggest is probably a good idea.
tests/collection.js
Outdated
|
|
||
| assertItems( collectionA, [ 4, 6, 8 ], [ [ 2, 4 ], [ 3, 6 ], [ 4, 8 ] ] ); | ||
| assertItems( collectionB, [ 2, 3, 4 ], [ [ 4, 2 ], [ 6, 3 ], [ 8, 4 ] ] ); | ||
| assertItems( collectionC, [ -2, -3, -4, -1000 ], [ [ 2, -2 ], [ 3, -3 ], [ 4, -4 ] ] ); |
There was a problem hiding this comment.
Aaa! Magic numbers. @scofalik would love it ;) But beeing serious, I wanted to write that these assertions should be split into separate assertItems and assertMapping what would make them more readable, but, as I wrote, I don't think mapping should be tested, but only the result of the mapping.
There was a problem hiding this comment.
Maybe you're right. This helped me a lot during development but shouldn't necessary remain in the tests.
…oExternalMap to WeakMaps. Removed internal assertions in tests.
…llection#_bindToExternalToInternalMap.
Suggested merge commit message (convention)
Feature: Two–way data binding between Collection instances. Closes ckeditor/ckeditor5#4939.