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

Mechanism for coercing non-records #38

Closed
matthewp opened this issue Aug 16, 2019 · 27 comments
Closed

Mechanism for coercing non-records #38

matthewp opened this issue Aug 16, 2019 · 27 comments

Comments

@matthewp
Copy link

matthewp commented Aug 16, 2019

What happens if you do:

const rec = #{ 
  date: new Date()
}

Does it call .valueOf() which in this case returns a Number?

@rickbutton
Copy link
Member

rickbutton commented Aug 16, 2019

It will not call valueOf in this case. It will instead result in a TypeError because the Date object cannot be used as a value in a const value type. See this section in the readme for the blurb (we could probably make this a bit more clear!)

Does this answer your question? If so, please feel free to close :)

@matthewp
Copy link
Author

It answers my question but I'm not sure I agree! :) . What is the justification for not calling .valueOf()? I think this breaks language consistency.

@bmeck
Copy link
Member

bmeck commented Aug 16, 2019

@matthewp I don't understand the language consistency argument. Can you explain where other cases exist that are calling .valueOf() in similar situations.

@matthewp matthewp changed the title is valueOf called? Implicit coercision Aug 16, 2019
@matthewp matthewp changed the title Implicit coercision Mechanism for coercing non-records Aug 16, 2019
@matthewp
Copy link
Author

I've renamed the title. I don't want this to be about whether valueOf() specifically is the right answer, but to the general question about whether/how non-Record objects can be coerced to types that records accept. In a follow-up comment I'll post some cases.

@rickbutton
Copy link
Member

FYI, this topic (whether to allow non-immutable types be values in Record/Tuple) is already tracked in #31

@matthewp
Copy link
Author

Thats not what this topic is about.

@rricard
Copy link
Member

rricard commented Aug 16, 2019

@rickbutton it's a different topic effectively, the question here is around calling valueOf or not

@rickbutton
Copy link
Member

Ah yes, good point. #31 is more about "should objects be allowed in records" and this is more about "how to coerce objects into values that can fit into records", good point!

@mheiber
Copy link
Contributor

mheiber commented Aug 17, 2019

This part of the proposal shows the usage of Record.fromEntries and Tuple.from: https://github.com/rricard/proposal-const-value-types#instantiation-and-converting-from-non-const-types

I imagine Record.fromEntries will work similarly to Object.fromEntries. So in this case, we'd get something like:

assert.equal(
    Record.fromEntries([["date", new Date().toUTCString()]]),
    #{ date: "Sat, 17 Aug 2019 19:19:42 GMT" }
);

@mheiber
Copy link
Contributor

mheiber commented Aug 20, 2019

@matthewp can we close this issue?

@matthewp
Copy link
Author

@mheiber No, fromEntries / from only do shallow conversion so they won't work for nested data structures. If this proposal supported some mechanism like valueOf or a symbol then the issue would be resolved.

@mheiber
Copy link
Contributor

mheiber commented Sep 9, 2019

Deep conversion could be implemented in user land on top of shallow conversion. It's likely to be expensive, so I worry about encouraging deep conversion.

valueOf is an interesting idea. Would you expect it to return an object such that its prototype is Object?

@littledan
Copy link
Member

I see two separate issues here:

  • Deep conversion (e.g., of nested objects and arrays to nested records and tuples) is a potentially useful, but expensive, operation that we might want to expose. Right now, there's a note in the README alluding to this. I'm happy with its omission in this draft, since it'd be good to avoid people doing too much conversion back and forth (as sometimes occurs in Immutable.js usage)
  • Implicit coercion with (presumably) ToPrimitive, for things in tuple and record literals. I just don't understand the motivation for this. I think it'd be best for developers if we threw an exception here. No other collection literal does implicit coercions, so it seems pretty surprising to me.

@matthewp
Copy link
Author

matthewp commented Sep 19, 2019

@littledan On the second point, is your issue specifically with the literal form or does it apply to from / fromEntries as well?

edit: rereading your first point makes me think that your issue is with the literal form.

@littledan
Copy link
Member

I would be surprised if we called ToPrimitive in either case.

@matthewp
Copy link
Author

matthewp commented Sep 20, 2019

Not sure if ToPrimitive would be the right mechanism or something else. Often the use case is to convert an object to a record. Is a record a primitive?

Whatever the mechanism, if we're talking about from and not just the literal form, then one precedent for something like this is toJSON.

@littledan
Copy link
Member

In general, I think records and tuples need to be treated as "primitives". So ToPrimitive would return the same record or tuple.

I don't think toJSON is much precedent here. That's a really specific serialization. This is a general construct within the language. I think it'd be pretty odd for us to start calling methods in these cases; at the same time, if we start by throwing an exception, it may be possible for a follow-on proposal to call a method for the case.

@littledan
Copy link
Member

I'd like to propose that the current state of the proposal is a good conclusion for this issue: You can construct Records with Record({ ... }) and Record.fromEntries([...]), and this does not call ToPrimitive.

@rricard
Copy link
Member

rricard commented Jan 21, 2020

Agreed, @matthewp please let me know if you want to add anything, otherwise I'm probably close the issue soon

@matthewp
Copy link
Author

I disagree with the outcome here. I would like to create APIs that return objects and have those be serializable to Records transparently, without the consumer of the API being aware of every (nested) property on that object and having to do it manually themselves.

We have this same capability with JSON today. Without something like this we will need to create toRecord() methods on our return objects, which I suspect is what's going to happen.

@rricard
Copy link
Member

rricard commented Jan 21, 2020

At the moment we're trying to edge towards being explicit: what should be the behavior when you plug in an object there? TypeError, it's predictable and easy.

Nothing prevents you to create a function that ensures serialization:

function s(objOrValue) {
    if (typeof objOrValue === "object" && "toPrimitive" in objOrValue) {
        return objOrValue.toPrimitive();
    }
    return objOrValue;
}

class FooBar {
    constructor(foo, bar) {
        this.foo = foo;
        this.bar = bar;
    }
    toPrimitive() {
        return #{
            foo: this.foo,
            bar: this.bar,
        };
    }
}

const foobar = new FooBar(1, 2);

const meta = #{ message: "it's a foobar" };

const foobarContainer = #{
    value: s(foobar),
    meta: s(meta),
};
console.log(foobarContainer);

Playground Link

@rricard
Copy link
Member

rricard commented Jan 21, 2020

I don't want to dismiss your use case but I'm not entirely sure we want to make it hard to explain why it does a TypeError sometimes and sometimes not because the object is serializable...

@matthewp
Copy link
Author

matthewp commented Jan 21, 2020

At the moment we're trying to edge towards being explicit: what should be the behavior when you plug in an object there? TypeError, it's predictable and easy.

This is where we disagree, it's not predictable if in some places the language requires explicitness and in some places the language is implicit. It becomes quite unpredictable, which makes the language more difficult to use overall. JavaScript has been an implicit language in a number of ways over the years from type coercion to toJSON. Additions to JavaScript should not relitigate whether or not those were good choices but should just follow the language for what it is.

Nothing prevents you to create a function that ensures serialization:

Since we know that such serialization functions are going to be written and bulk up bundle sizes, why not just build this useful feature into the language?

@littledan
Copy link
Member

@matthewp Is there anything more you could say about kinds of use cases you're thinking of, and why @rricard 's solution doesn't work well for it? Is this just about bundle size, or are there further reasons? (ergonomics?)

@rickbutton
Copy link
Member

Given the solution posed in #38 (comment) , and the language's history of not providing a built-in deep-copy/clone operation for objects, I think the conclusion here should be that the methods of coercion to Record in the explainer (Record.from etc...) are satisfactory. If there is significant desire for a deep-copy/clone/coercion operation for Record (and object?), it can be easily iterated on in a separate proposal. If there are no objections I would like to resolve this issue with that conclusion.

@matthewp
Copy link
Author

matthewp commented Mar 4, 2020

No objection.

@rickbutton
Copy link
Member

Great! Thank you all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants