Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support init or setup returning something other than new instance #30

Merged
merged 5 commits into from
Jan 26, 2017

Conversation

ilyavf
Copy link
Contributor

@ilyavf ilyavf commented Jan 24, 2017

Allow to return special new Construct.ReturnValue( value ) from setup method, so that this value will be returned as the result of new Construct( ... ):

var Student = function(){};
var Person = Construct.extend({
    setup: function(params){
        if (params.isStudent){
            return new Construct.ReturnValue( new Student(params) );
        } else {
            return [params];
        }
    }
});
console.log( new Person({isStudent: true}) instance of Student );
// => true

@ilyavf ilyavf force-pushed the issue/2/new-instance-return-value branch from 5aa7e69 to 69a5b1d Compare January 24, 2017 19:57
@ilyavf
Copy link
Contributor Author

ilyavf commented Jan 24, 2017

Fixed acc to feedback. Need to add docs...

@ilyavf ilyavf force-pushed the issue/2/new-instance-return-value branch from 949a352 to 45d4bc9 Compare January 24, 2017 23:06
* var item = myStore[params.id];
*
* // Merge new data to the existing instance:
* Object.keys( params ).forEach(function( key ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably use Object.assign here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* }
* },
* init: function(params){
* this.id = params.id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might use Object.assign here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* this.name = params.name;
*
* // Save to cache store:
* myStore[this.id] = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should almost certainly be done in setup. init is usually left for people inheriting from Item to use for their particular Item type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* @function can-construct.ReturnValue ReturnValue
* @parent can-construct.static
*
* A constructor function which `value` property will be used as a value for a new instance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much detail for a description. It shouldn't care at all about the value property. Maybe something like:

Use to overwrite the return value of new Constructor(...).

* A constructor function which `value` property will be used as a value for a new instance.
*
* @signature `new Construct.ReturnValue( value )`
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature should talk about how this works. Also, the example code below should be put in the signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*
* @signature `new Construct.ReturnValue( value )`
*
* @param {*} value A value to be used for a new instance instead of a new object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the value MUST be an Object type. If someone tries to return a number, it won't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, indent 2 spaces @param and other nested @ tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* arguments to [can-construct::init init]. The following example always makes
* @return {Array|undefined|can-construct.ReturnValue} If an array is returned, the array's items are passed as
* arguments to [can-construct::init init]. If [can-construct.ReturnValue] is returned then the value that passed to it
* will be used as a value for a new instance. The following example always makes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change:

If [can-construct.ReturnValue] is returned then the value that passed to it
will be used as a value for a new instance.

To something more like:

If a [can-construct.ReturnValue] instance is returned, the ReturnValue instance's
value will be returned as the result of calling new Constructor().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@ilyavf ilyavf force-pushed the issue/2/new-instance-return-value branch from 3350f68 to 87b671e Compare January 26, 2017 18:35
@ilyavf ilyavf merged commit e4cd3c3 into master Jan 26, 2017
@ilyavf ilyavf deleted the issue/2/new-instance-return-value branch January 26, 2017 18:37
@chasenlehara chasenlehara changed the title Issue/2/new instance return value Support init or setup returning something other than new instance Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants