Inconsistent field type default method invocation

[adwydman]

I'm adjusting field-type.test.js and I found inconsisten use of the default field-types method invocation:

it("should use default .is_proper_value", function(done){
    const result = test_field_type.is_proper_value("any_value");
    assert_no_error(result, done);
});
it("should use default .is_proper_declaration", function(){
    var return_value = test_field_type.is_proper_declaration({});
    assert.strictEqual(typeof return_value, "boolean");
    assert.strictEqual(return_value, true);
});
it("should use default .encode", function(done){
    test_field_type.encode(new Context(), {}, "dogfood") // this is a Promise and for example `.decode()` is not
    .then(function(encoded){
        assert.strictEqual(encoded, "dogfood");
        done();
    }).catch(done);
});
it("should use default .decode", function(){
    const decoded = test_field_type.decode(new Context(), {}, "dogfood")
    assert.strictEqual(decoded, "dogfood");
});
it("should use default .get_description", function(){
    const description = test_field_type.get_description()
    assert.strictEqual(description.summary, "test_field_type");
});

Is this how they are meant to work?

[kuba-orlik]

What inconsitencies do you have in mind?

[adwydman]

That encode() returns a Promise and the rest, like decode() or get_description() returns a value. Is there any reason for encode() to return a Promise and for the rest of the methods not to?

[kuba-orlik]

I think they were supposed to support both synchronous (return) and asynchronous (return Promise) flows.

[adwydman]

Support both? Maybe let's switch to Promises only?

[kuba-orlik]

Yeah, I guess we should favor consistency over convenience here

[kuba-orlik]

Hmmm, on second thought: ResourceType converts both synchronous and Promise-based returns into a promise, with this method:

    this.decode_values = function(context, values){
        const decoded_values = {};
        for (const key in this.fields){
            const value = values[key];
            const field = this.fields[key];
            if (field === undefined){
                continue;
            }
            decoded_values[key] = field.decode(context, value);
        }
        return Promise.props(decoded_values);
    };

(https://github.com/sealcode/sealious/blob/remove_unnecessary_dependencies/lib/chip-types/resource-type.js#L241)

So it doesn't cost us much complexity to support both options. Maybe we should keep it that way?

[adwydman]

Well then what's the point of having promises in the default methods?

[kuba-orlik]

I guess there's none (at least in case of encode/decode) and I think it can be safely turned into a synchronous return. I'm just thinking whether we should change the API to promise-only or not.

[kuba-orlik]

@adwydman (in response to your ping from https://github.com/sealcode/sealious/pull/285#issuecomment-240057281): I think we should leave the API to allow both synchronous (return) and asynchronous (return Promise.resolve) returns. As both these ways of returning are currently supported, I don't see the need to stick to only one option in the default methods. I do think, however, that default methods for those field types should only use Promise.resolve when dealing with acynchronicity, so all methods that can be synchronous, should be synchronous.

[adwydman]

const default_methods = {
    is_proper_value: function(context, params, new_value, old_value){
        return Promise.resolve();
    },
    format: function(context, params, decoded_value, format_params){
        return Promise.resolve(decoded_value);
    },
    encode: function(context, params, value_in_code){
        return Promise.resolve(value_in_code);
    },
    get_description: function(context, params){
        return new FieldTypeDescription(this.name);
    },
    decode: function(context, params, value_in_database){
        return value_in_database;
    },
    filter_to_query: function(context, params, query){
        return Promise.resolve(this.encode(context, params, query))
        .then(function(encoded_value){
            return {
                $eq: encoded_value,
            };
        });
    },
    full_text_search_enabled: function(){
        return false;
    }
};

Which methods deal with asynchrony? Only filter_to_query?

[kuba-orlik]

I think they all should be able to return both synchronously and asynchronously. Of course there's no asynchronous behavior in the placeholder, default methods, so there's no need for asynchronous overhead here.

[adwydman]

But if they are wrapped in a promise anyway, then why can't they just straight up return values in a synchronous manner?

[kuba-orlik]

I meant: in general, field-type methods should be allowed to return both synchronously and asynchronously. I agree. The default methods should not create an additional asynchronous overhead and just return synchronously. :)

[adwydman]

https://github.com/sealcode/sealious/pull/295