Simplified example with tests and spec changes. #18
Conversation
| const composable = {}; | ||
| const methods = getDescriptorProps('methods', composables); | ||
|
|
||
| composable.compose = (...args) => {}; |
There was a problem hiding this comment.
Obviously need to implement this. Just trying to illustrate a more self-documenting way to handle the descriptor property mixing for now...
|
I am agree to any changes to the example-implementation as long as it's highly readable by other people. The intention was to make sure any other person can read the code, easily understand it and (re)implement. If you consider this example code should serve real production - then let it be. |
That's what I'm aiming for. I'll do some more work on it this weekend. =) |
| }; | ||
|
|
||
| function compose (...composables) { | ||
| const composable = {}; |
There was a problem hiding this comment.
I thought composable is a factory function. :)
There was a problem hiding this comment.
Yeah, these are just placeholders. Like I said, I haven't implemented the stamp bit yet. :)
There was a problem hiding this comment.
Yes, sure. :) Seem like you respond in Gitter faster than on GitHub.
* Instance creation * Initializers run * Better initializer tests * New watch task
* Add eslint * Cleanup lint
|
|
||
| // Set up prototype | ||
| Object.assign(proto, descriptor.methods); | ||
| Object.setPrototypeOf(obj, proto); |
There was a problem hiding this comment.
You'd need to check if the object already have a prototype. It should not be overriden.
if (Object.getPrototypeOf(obj) === Object.prototype)There was a problem hiding this comment.
Yep. Doing TDD, meaning that I write the minimum code to make the test I'm working on pass.
There was a problem hiding this comment.
We should avoid making changes to an object's [[Prototype]]. Instead we can just create a new object.
const proto = {};
let obj = instance || {};
// Set up prototype
Object.assign(proto, descriptor.methods);
if (isEmpty(Object.getPrototypeOf(obj))) {
obj = Object.create(proto);
}http://jsperf.com/proto-vs-object-create2/2
Before you say it @koresar, I don't consider this a premature optimization. ;)
There was a problem hiding this comment.
Yeah, I'm using Object.create() where it's possible, but if a user passes an instance in to be mutated, we can't just Object.create() a new instance. For example, a user might want to use an array or a function, and Object.create() won't work for those cases.
There was a problem hiding this comment.
Good call, I overlooked that scenario.
* Instance prototype handling * Pass in stamp * Silent watch task
| nest.test(`...with pojo descriptor.${ descriptorName }`, assert => { | ||
| const descriptor = { | ||
| [ descriptorName ]: { | ||
| a: 'a' |
There was a problem hiding this comment.
This will throw a TypeError for staticPropertyDescriptors when Object.defineProperties is used in createStamp.
TypeError: Property description must be an object: a
This also applies to all other test files that test property descriptors.
There was a problem hiding this comment.
I think I'll try to got some of the object-instantiation stuff worked out today, so I'll probably run into this soon. =)
Since it would be trivial to implement `instance` mutation as a stamp, it makes sense to strip it from the stamp spec to simplify things and eliminate any need for reserved properties. I believe it's still a very good idea to encourage the use of a single `options` object, so that will remain part of the spec.
…o ericelliott-simplify-example
ericelliott
changed the title
|
This looks good to me. 👍 |
|
Good quality code. Awesome Eric. :) Tests are comprehensive. I compared them to stampit tests and find that you have covered everything. I might doubt about some wrding in the README, but it's only a matter of taste. |
Simplified example with tests and spec changes.
Simplified example implementation with tests.
Includes spec changes discussed in #35.