mobxjs / serializr

Serialize and deserialize complex object graphs to and from JSON and Javascript classes
MIT License
766 stars 52 forks source link

Is deserialize forced with 3 parameters (class, json, callback)? #54

Closed yamsellem closed 7 years ago

yamsellem commented 7 years ago

Hi,

I'm having a hard time with the deserialize function. If I call it with two params (as documented), it throws a callback is not a function at deserializeObjectWithSchema (node_modules/serializr/serializr.js:487:29).

Here is a code, with, in comments, the fix who seems to fix the issue.

const test = require('ava');
const { createModelSchema, primitive, serialize, deserialize } = require('serializr');

class Todo {
    constructor(body) {
        Object.assign(this, deserialize(Todo, body)); // throws callback is not a function

        /* has to be changed to that in order to work:
         *
         * Object.assign(this, deserialize(Todo, body, () => {})); 
        */
    }
}

createModelSchema(Todo, {
    name: primitive()
});

test.only('deserialize', t => {
    const todo = new Todo({ name: 'things' });
    t.is(todo.name, 'things');
});

Thanks for your help.

alexggordon commented 7 years ago

So I believe your issue is that the deserializeis self referential. Deserialize under the hood still calls new Todo(), which is your issue.

In your test, what you need to do is actually just remove the constructor.

const test = require('ava');
const { createModelSchema, primitive, serialize, deserialize } = require('serializr');

class Todo {
    name;
}

createModelSchema(Todo, {
    name: primitive()
});

test.only('deserialize', t => {
    const todo = deserialize(Todo, { name: 'things' }))
    t.is(todo.name, 'things');
});

(I haven't tested that code, but it should work)

Alternatively, you can use the decorators in serializer too.

const test = require('ava');
const { createModelSchema, primitive, serialize, deserialize, serializable } = require('serializr');

class Todo {
    @serializable name;
}

test.only('deserialize', t => {
    const todo = deserialize(Todo, { name: 'things' }))
    t.is(todo.name, 'things');
});
yamsellem commented 7 years ago

Thanks for your quick answer.

I cannot use decorators yet.

And I don't get it: removing the constructor? But that the essence of a class. Do you mean that this library cannot be used with a constructor?

I was worried with the way it wraps an object. Wrapping outside of the class is not really object oriented. May I suggest allowing to wrap an object within its constructor?

Even better: don't wrap it at all. Use a custom property in the class – say 'serializables' or 'fields' – that define the schema (a configurable property name, maybe). Thus, a class can define what attributes should be serialized without that global wrapping declared outside.

-

Last, your code is not es6 JavaScript, is it?

class Todo { name; }

This code does not works on node 6.9. I've never seen this syntax, though.

Thanks again.

alexggordon commented 7 years ago

@yamsellem what I mean is that you can't deserialize a class, to itself, in the constructor of the object.

You can use the constructor of your class to do really whatever you want, but that's not where you should be deserializing the class itself.

As far as wrapping the object, it does that to provide some state to the javascript object. You can see that it adds a couple attributes to the class to allow for serialization/deserialization.

Wrapping an object in the constructor would allow for too many possible issues, and most likely won't be a feature that's added.

Finally, that is valid es6 javascript. It's actually using the stage 3 class fields, so you'd need that babel plugin to compile.

alexggordon commented 7 years ago

closed for inactivity