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

Consider removing toString and valueOf methods #74

Closed
littledan opened this issue Jul 27, 2018 · 40 comments
Closed

Consider removing toString and valueOf methods #74

littledan opened this issue Jul 27, 2018 · 40 comments
Labels
behavior Relating to behavior defined in the proposal spec-text Specification text involved
Milestone

Comments

@littledan
Copy link
Member

Various parts of JS call these operations implicitly, when converting to a Number or String. Enabling such implicit conversions doesn't mesh well with the strong typing of this proposal. I am wondering, would it make sense to expose this functionality under other names, which don't enable coercion, such as toISOString and nanosecondsSinceEpoch? It would make some code more wordy, but the upside would be less risk of errors.

@pipobscure
Copy link
Collaborator

Ok.

Questions:

toString() isn't actually equivalent to toISOString() because the output is a guaranteed subset of ISO. Is there a better name? Especially since the reverse fromString() should really not be named fromISOString() since that implies submitting any ISO-String would be accepted and we don't want to do that.

valueOf() is designed to produce a BigInt right now, so that's a pro for your comment. However? What should the valueof() then be?

Also I really dislike having both nanoseconds and nanosecondsSinceEpoch the (me personally, I think the nanoseconds value should result in the full nanoseconds since epoch rather than just the nanosecond part). If that's the case, then I'd be good with removing valueOf().

@ljharb
Copy link
Member

ljharb commented Jul 27, 2018

toString is also frequently used for debugging - not just for implicit coercions.

I would not be comfortable with repeating the (necessary and understandable yet still) highly unergonomic situation of Symbol’s undebuggability, nor with “[object Object]”.

@littledan
Copy link
Member Author

@ljharb Could you give more information on what you would like to see from debugging Temporal objects? (Presumably the alternative is, e.g., [object Instant], etc)

@ljharb
Copy link
Member

ljharb commented Jul 28, 2018

An ISO timestamp would be sufficient, although also including the type would be excellent.

@littledan
Copy link
Member Author

@ljharb I mean, what sort of debugging flows are you thinking about? Overall, toString isn't great for reliably serializing an object for debugging--look at its weird output for Arrays ([1].toString() === "1"), which we carried forward for TypedArrays. For this reason, lots of development environments support other ways of displaying objects (e.g., in DevTools consoles).

@littledan
Copy link
Member Author

@pipobscure I don't have a strong preference on either of those questions; the names I was suggesting were just an early draft suggestion. What I'm trying to get at is, if you use those special names, special things will happen, and I'm not sure if that's what we want.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2018

True, things like https://npmjs.com/object-inspect combine the toString output with type output; but it’d be nice to not have to jump through hoops.

@littledan
Copy link
Member Author

@ljharb Is there anything you could tell me about your debugging flow that creates this requirement?

@ljharb
Copy link
Member

ljharb commented Jul 28, 2018

I’m not sure what explanation you’re looking for - i have a value (in a debugger or a logging statement or an exception message or a test framework assertion), instanceof isn’t reliable/cross realm, and i want the quickest and most generic path to a useful string that describes the value.

@keithamus
Copy link
Member

Almost every platform you can use JS on does deeper introspection of a value than just toString(). Many new ES object types including WeakMap, WeakSet, Map, Set, Promise etc expose no useful toString() or valueOf() value, but play just fine when debugging via console.log in Browsers and Node. If your testing framework does not do the same, then perhaps that is where this should be fixed.

I'm personally very much for dropping toString() and valueOf() here.

@maggiepint
Copy link
Member

I don't have a strong opinion on .toString(), but with some thought .valueOf() being the string subsets of ISO we have been discussing makes (to me) a lot of sense. Because the string subsets are unique on a per-type basis, comparison and sorting functions will work pretty much exactly as expected if we use the string values.

@littledan
Copy link
Member Author

valueOf is actually the more problematic one. It means that comparison doesn't just work within a type but between types. Is this what you really want?

@maggiepint
Copy link
Member

I suppose equality will work, but comparison is still broken.

@littledan
Copy link
Member Author

What do you mean? If we remove valueOf, the meaning of < goes from comparing ns to throwing a TypeError, right? Then, we would add comparison methods.

@pipobscure
Copy link
Collaborator

I would actually vote for removing valueOf() entrirely. It makes things unclear because:

let a = { valueOf: function() { return 3; } };
let b = { valueOf: function() { return 4; } };

b == 4; // true
b.valueOf(); // 4
a < 4; // true
a < b; // false

That to me make valueOf() one of the bad parts™️. This holds especially true since typeof instant.valueOf() === 'bigint'.

pipobscure added a commit to pipobscure/proposal-temporal that referenced this issue Aug 2, 2018
@pipobscure
Copy link
Collaborator

In #76 I changed

  • .toString() to give [object Type ISO-String] - which is good for debugging from log-files where they are stringified.
  • .asString() to give ISO-String - (namecheck) to be what .toString() used to be
  • .valueOf() became a .value getter that returns a BigInt

Comments Please!!!

@domenic
Copy link
Member

domenic commented Aug 2, 2018

-1 for toString giving somethign of the form [object Type X]. That is unprecedented in JS and not a good pattern in general. I think it should just do what it does for every other type, and give a useful string representation, e.g. ISO-String.

@ljqx
Copy link
Contributor

ljqx commented Aug 3, 2018

How about

  1. for CivilDate
  • CivilDate.prototype.[to/from]ISOCalendarDateString => YYYY-MM-DD => 1981-04-05
  • CivilDate.prototype.[to/from]ISOWeekDateString => YYYY-Www-D => 2009-W01-1
  • CivilDate.prototype.[to/from]ISOOrdinalDateString => YYYY-DDD => 1981-095

"calendar date", "ordinal date", "week date" are concepts defined by the specification.

  1. CivilTime
  • CivilTime.prototype.[to/from]ISOTimeString => hh:mm:ss.mmmmmm
  1. CivilDateTime
  • CivilDateTime.prototype.[to/from]ISODateTimeString => YYYY-MM-DDThh:mm:ss.mmmmmm

It's wordy though.

@pipobscure
Copy link
Collaborator

@ljqx the reason I dislike the toISO*** method names is because there is a suggestion of being ISO complete. This comes back to bite us when we define fromString() because rather than having a full fledged parser that allows all ISO (and other) string configurations, we went with this one because we defined it with reference to toString() and said that all i can parse is the format produced by that.

Of course we could go whole hog on the pairs:

  • toISOCalendarString() -> fromISOCalendarString()
  • toISOWeekDateString() - fromISOWeekDayString()
  • toISOOrdinalString() - fromISOOrdinalString()
  • toISOTimeString() - fromISOTimeString()
  • toISODateTimeString() - fromISODateTimeString()
  • toISOWeekDateTimeString() - `fromISOWeekDateTimeString()
  • toISOOrdinalTimeString() - fromISOOrdinalTimeString()
  • ...

The question is whether there is any real benefit; also would string formatting not be better left to something other such as Intl.DateTimeFormat and the like.

@pipobscure
Copy link
Collaborator

The conclusion I draw from the discussion so far is:

  • toString() is a good thing and should output a plain ISO-String
  • valueOf() is a bad thing and should not exists on temporal objects
  • .value can be the member with the nanosecond-since-epoch as a BigInt
  • .toJSON() should act like Date and output the ISO-String

(Please up/down - vote to indicate whether my summary is accurate or if it's premature)

@gibson042
Copy link
Collaborator

valueOf() is a bad thing and should not exists on temporal objects

There can be a pretty big difference between valueOf absent vs. identity vs. throwing. Maybe this should be considered in terms of effects upon relational comparison and equality comparison between temporal values and strings/Numbers/BigInts/other temporal values (keeping in mind that @@toPrimitive can preempt its invocation).

Use of relational operators to compare at least date-times with each other and dates with each other might be nice enough to justify non-abrupt completions from cross-type comparisons (even though those results would be meaningless), which in concrete terms would probably look like ToPrimitive(CivilDateTime) and ToPrimitive(CivilDate) returning BigInt (implicitly also supporting both relative and equality comparison against numeric values) and ToPrimitive(CivilTime) throwing.

@littledan
Copy link
Member Author

+1 to #74 (comment) re valueOf and toString, except:

  • value is probably worth another round of bikeshedding (shouldn't this name somehow say what it represents?)
  • I am not sure if we want to add another toJSON method by default which doesn't round-trip; I am not sure if Date is a model we want to repeat

@pipobscure
Copy link
Collaborator

pipobscure commented Aug 5, 2018 via email

@littledan
Copy link
Member Author

Well, I think we should come to a common conclusion among Temporal and BigInt re the throwing issue. BigInt could also serialize 123n as "123" and require using the BigInt constructor to revive it. However, doing this properly requires understanding the schema. There's an active thread on this question at tc39/proposal-bigint#162 . Last time this was discussed, re Map and Set, the conclusion was to continue to not give useful output; see tc39/proposal-bigint#162 (comment)

@kaizhu256
Copy link
Contributor

throwing could break loggers that rely on JSON.stringify/toString (which until now, have assumed vanilla objects/builtins/primitives won't throw), like this real-world example:

/*
 * json-logging example from
 * https://github.com/kaizhu256/node-utility2/blob/2018.6.21/lib.utility2.js#L4784
 */
// debug middlewareForwardProxy
console.error('serverLog - ' + JSON.stringify({
    time: new Date(options.timeStart).toISOString(),
    type: 'middlewareForwardProxyResponse',
    method: options.method,
    url: options.url,
    statusCode: response.statusCode | 0,
    timeElapsed: Date.now() - options.timeStart,
    // extra
    headers: options.headers
}));

@gibson042
Copy link
Collaborator

I raised the possibility of throwing from valueOf, not from toString or toJSON. The effects of which would be similar to Symbols, for which Abstract Relational Comparison also throws (albeit a little later, at the application of ToNumber rather than ToPrimitive).

@pipobscure
Copy link
Collaborator

Summary:

  • .toString() - generates plain ISO-Strings
  • JSON.stringify() - acts like Date (uses .toString())
  • no .value or .valueOf()

Can we take this as sort-of agreed? (I'm about to start updating things in advance of the TC39 meeting to reflect the current state)

@maggiepint
Copy link
Member

This is agreed. Going to note this when I present.

@littledan
Copy link
Member Author

JSON.stringify() - acts like Date (uses .toString())

This is the part where I don't really agree--I'd prefer we throw an exception or generate { }. Date is not a precedent to be followed by default, as it fails to round-trip.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2018

{} doesn't round-trip either.

@littledan
Copy link
Member Author

@ljharb I'm not sure what you mean, or how that relates to what we should do here.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2018

You said "I'd prefer we throw an exception or generate { }", and that you didn't want to follow Date because it does not round trip, but "generate { }" does not round trip either.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2018

In other words, if round tripping is a requirement, then throwing an exception is the only option. However, I don't think there's consensus that round tripping is a requirement.

@littledan
Copy link
Member Author

I see, well, I'd say it's better to either fail somewhat loudly/thorougly or be really correct; muddling through makes me a bit worried. In particular, about JSON, we discussed this issue with respect to Map and Set, and came to the conclusion that they should continue to return { }, so I'd like to understand what's different here than in that case. (Maybe they are the same and we should actually revisit the Map/Set proposal.)

@pipobscure
Copy link
Collaborator

pipobscure commented Oct 9, 2018

The difference is that Map and Set are collections. So there could be arbitrarily nested stuff inside so that there is little to no chance of reconstituting the collection.

On the contrary, the temporal objects all constiture single discreet values, and the objects come with a static function (fromString) that can be used to reconstitute them using the established reviver hook.

In addition, the readable ISO string format actually has data value and works transparently, making multiple round-trips painless.

In my opinion this is the one thing that Date actually got right.

@kaizhu256
Copy link
Contributor

agree with @pipobscure. non-nested data-structures should follow Date's JSON behavior for ease-of-integration with JSON and consistency.

i'm skeptical general-purpose tooling to auto-revive JSON data-structures without application-context will ever be viable. its naive thinking. javascript product development will always require user-intervention in the JSON.parse/revival step. the best tradeoff is too accept that and focus at least on making JSON.stringify automatic.

@littledan
Copy link
Member Author

OK, I take it you all disagree with the decision of BigInt to not use toString as its toJSON, then?

@pipobscure
Copy link
Collaborator

pipobscure commented Oct 10, 2018

@littledan most definitely. It makes working with BigInt needlessly hard.

All it did is require me to have a mini library:

export const replacer = (k,v)=>('bigint'===typeof v)?(v+'n'):v;
export const reviver = (k,v)=>/^\d+n$/.test(v)?BigInt(v.slice(0,-1)):v;

used like

JSON.parse(JSON.stringify({ a: 123n }, replacer),reviver)

just making it needlessly painful for no benefit. It's not like throwing on JSON.stringify made anything work better.

@littledan
Copy link
Member Author

OK, in this case, I think we should come up with a common decision for both. We were pretty explicit about this decision for BigInt, but it could be revisited without (probably) breaking compatibility.

@pipobscure
Copy link
Collaborator

obsolete. Conclusion: no valueOf but yes on toString

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

10 participants