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 to exclude methods when extracting toJSON() #79

Merged
merged 2 commits into from Jun 27, 2016

Conversation

BWorld
Copy link

@BWorld BWorld commented Jun 24, 2016

Hi,

I think this project is awesome but I bumped into a small issue which I am creating this PR for.
When extracting a model into JSON it is currently not (easily) possible to exclude methods.

I added an option which can disable the extraction of methods and only extract properties defined in the schema.

Happy to receive feedback.

Chris Blokland added 2 commits June 24, 2016 13:31
At first it was impossible excluding methods when extracting data to
json (toJSON()). A new property can be set to true which is called
'_extractDocPropertiesOnly'

For BC reasons it is by default turned on to include methods as well.
@scottwrobinson
Copy link
Owner

Thanks for the PR! I just ran in to this problem recently as well. Guess I didn't think through the original implementation very well.

I'm thinking methods should always be excluded when serializing with toJSON(), or at least excluded by default, since this will be the vast majority of use-cases. I can see some cases where people will need to serialize functions and regexps, but that will require some extra work (see serialize-javascript). For now, I think we just exclude all methods.

@scottwrobinson scottwrobinson merged commit 2ec0e9e into scottwrobinson:master Jun 27, 2016
@BWorld
Copy link
Author

BWorld commented Jun 28, 2016

Hi Scott,

Thanks for your response.
I implemented this as an option for keeping BC. Should it stay there until a major release is done?

Another thought I was having was that maybe introducing some extraction / hydration interface to the document instances will make it more flexible and documents more re-usable. i.e toJSON() proxies to a new method extract('json')

When you have input data you can easily hydrate it into your document instance using hydrate(data, 'ApiInput') for example because data hydrated from the mongo storage accepts more data than from some api endpoint for example. Same go's for outgoing data (extraction).

A code example:

const person = new Person();
person.hydrate({
    a : 123,
    b : 'abz'
}, 'foo');

class Person extends Document {
    hydrate(data, context) {

    }

    extract(context) {
        switch(context) {
            case 'api':
                return {...} // will format things specificly for api responses
            case 'statistics':
                return {....} // will format things specificly for statistic overviews maybe
            case ...
        }
    }
}

Default behavior would be hydrating and extracting based on the document definition. (as how it is done at the moment).

This will be just a start, you could also register hydrators by id having a class instance as value so you can separate the logic for hydration/extraction from the document instance itself. When you do this you can easily build some default hydrator which can take strategies into account on a per field basis.

Hydrator interface would be something like this (pseudo code)

interface Hydrator {
   Object extract(Document doc);
   void hydrate(Object data, Document doc);
}

class DefaultHydrator implements Hydrator {
        // extraction and hydration is based on doc schema
    extract(Document doc) {

    }
    hydrate(Object data, Document doc) {

    }
}

These are just thoughts and ideas to make it more flexible.
Tell me what you think!

@scottwrobinson
Copy link
Owner

Thanks for the notes, Chris.

I was treating this as a bug, although you might be right that it should have been preserved for backwards compatibility.

As for the Hydrator, I like the idea, but I think it might be a bit out of scope for Camo. The same thing could be implemented in a base class and doesn't really need to be in Camo's Document.

However, I've been wanting to implement a plugin system in Camo, and I think the Hydrator would be perfect for that.

Let me know what you think. Thanks for the ideas!

@BWorld
Copy link
Author

BWorld commented Jun 29, 2016

Hi Scott,

I will let that up to you how you like to release the change.

I think you are right that it belongs inside a plugin that will provide
hydration and extraction per context.

For now, personally, I can do all the things with overriding the toJSON
method combined / setting schema properties to private.

But ofcourse there are many other things you can do with plugins and it
will be easier for others to contribute so +1 for plugins.

Are you still looking for contributors?

Chris.
Op 29 jun. 2016 04:42 schreef "Scott Robinson" notifications@github.com:

Thanks for the notes, Chris.

I was treating this as a bug, although you might be right that it should
have been preserved for backwards compatibility.

As for the Hydrator, I like the idea, but I think it might be a bit out of
scope for Camo. The same thing could be implemented in a base class and
doesn't really need to be in Camo's Document.

However, I've been wanting to implement a plugin system in Camo, and I
think the Hydrator would be perfect for that.

Let me know what you think. Thanks for the ideas!


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#79 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAwmH9cQT4bnmQSUNH0zmQn2LB1EUbJLks5qQduagaJpZM4I9s_0
.

@scottwrobinson
Copy link
Owner

Yes! I'm still looking for contributors. Are you interested? I need to get a bit more organized, but I think there is some interesting stuff to work on.

Let me know what you think.

Scott

@BWorld
Copy link
Author

BWorld commented Jun 30, 2016

Sounds good and yes I am interested.

Send me an e-mail with your ideas and then we discuss this further in
private.
My e-mail is: chris at dutchfrontiers dot com

Chris.
Op 30 jun. 2016 03:54 schreef "Scott Robinson" notifications@github.com:

Yes! I'm still looking for contributors. Are you interested? I need to get
a bit more organized, but I think there is some interesting stuff to work
on.

Let me know what you think.

Scott


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#79 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAwmH9tllXZ3RAK-mfzfTXNUzbtaxDdaks5qQyHrgaJpZM4I9s_0
.

@kurdin
Copy link

kurdin commented Sep 8, 2016

This update kills virtual properties getter to functional.

Now is this code, user object does not contain fullName key with value Billy Bob

class User extends Document {
                constructor() {
                    super();
                    this.firstName = String;
                    this.lastName = String;
                }

                set fullName(name) {
                    var split = name.split(' ');
                    this.firstName = split[0];
                    this.lastName = split[1];
                }

                get fullName() {
                    return this.firstName + ' ' + this.lastName;
                }
            }

            var user = User.create({
                fullName: 'Billy Bob'
            });

            console.log('user', user);

@kurdin kurdin mentioned this pull request Sep 8, 2016
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.

None yet

3 participants