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

Combined Parsing Convenience Method #244

Closed
maggiepint opened this issue Oct 31, 2019 · 45 comments
Closed

Combined Parsing Convenience Method #244

maggiepint opened this issue Oct 31, 2019 · 45 comments
Assignees
Labels
behavior Relating to behavior defined in the proposal enhancement

Comments

@maggiepint
Copy link
Member

We should consider a way to parse a string once, and output multiple objects. Like this:

Temporal.Parse('1900-01-01T00:00:00-01:00')

returns an object with properties:

{ 
Absolute: absolute, 
DateTime: datetime, //1900-01-01T00:00:00
Time : Time, // 00:00
Date: Date //1900-01-01 ,
Zone: Zone //fixed zone of negative one
/// and all the other types, or at least all the most common ones
}

This allows the string to only be parsed once, which is critical in some high-performance scenarios.

It might make sense to add an option to the parse parameter to specify which types are returned. Something like:

Temporal.Parse(Temporal.Parse('1900-01-01T00:00:00-01:00'), ['Date', 'DateTime', 'Absolute', 'Zone'])

I kinda hate the string array so ideas welcome there.

@mj1856 for reference.

@ryzokuken
Copy link
Member

/cc @littledan @pipobscure @Ms2ger

@littledan
Copy link
Member

I'm trying to understand the use case here. Is this issue sort of an effect of how we don't have a ZonedInstant type anymore? For an example like the one above, without a timezone, I don't see why we wouldn't use Temporal.DateTime.from[String], and then project the other types from there. If this is about parsing with a timezone, what if it returned an object with just a DateTime and tz string?

@ljharb
Copy link
Member

ljharb commented Oct 31, 2019

s/tz string/a TimeZone?

@pipobscure
Copy link
Collaborator

@littledan sort of. But your solution wouldn’t work, because DateTime + TimeZone is not guaranteed to exist and not guaranteed to be specific if it does. So you’d have to parse an Absolute as well. And then you are at this function.

@Ms2ger Ms2ger changed the title Combined Parsing Conveinence Method Combined Parsing Convenience Method Oct 31, 2019
@ljharb
Copy link
Member

ljharb commented Oct 31, 2019

If you have the Absolute, can you not cheaply derive the others from it?

@pipobscure
Copy link
Collaborator

@ljharb nope, because an Absolute enforces the TimeZone with the given offset. So there are situations where you can parse a TimeZone and a DateTime but not an Absolute. Plus with an Absolute you also loose the TimeZone.

@ljharb
Copy link
Member

ljharb commented Oct 31, 2019

If this is a perf consideration, couldn’t engines silently memoize their internal parsed representations, making repeat froms into different types faster? Does it need a user-exposed mechanism, forcing the user to handle it themselves?

@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal enhancement labels Nov 1, 2019
@ryzokuken ryzokuken added this to the November Meeting milestone Nov 1, 2019
@ryzokuken
Copy link
Member

Can we reach consensus on if this needs to be done?

@pipobscure
Copy link
Collaborator

Resolution: Yes

const { absolute, dateTime, timeZone, date, time } = Temporal.Parse(string);

@ryzokuken ryzokuken self-assigned this Nov 12, 2019
@maggiepint
Copy link
Member Author

Beautiful :-)

@littledan
Copy link
Member

Wait, I wanted to make a comment on the name: can this be lower case, matching all JS methods?

Separately, @sffc proposed making this an object with getters returning the Temporal types, to permit lazy allocation. I didn't hear any concerns raised.

@pipobscure
Copy link
Collaborator

pipobscure commented Nov 13, 2019

Agreed: it will be lowercased and the return will be an object with readonly properties. (whether that’s getters or just writable:false is an imp detail we should’t care about and can be done to optimise parsing. {it’s easy to convince me otherwise; any rationale will do})

@littledan
Copy link
Member

In specifications and implementations that try to be 100% conformant, getter vs read-only is not just an implementation detail. Do you have any concerns about the getter idea?

@pipobscure
Copy link
Collaborator

What does the getter do for missing parts? If it just returns undefined then ok, if it throws that would be bad.

@littledan
Copy link
Member

I was imagining it would return undefined.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2019

why would we need getters? the point of this api is to eagerly parse into objects for later usage - why not just normal writable data properties?

@sffc
Copy link
Collaborator

sffc commented Nov 13, 2019

The proposal requires instantiating 5 or more objects, when the user might only care for 2 of them, and the other three could be wasted. However, maybe that's more of a theoretical concern. The cost of parsing a date is probably at least an order of magnitude higher than a few superfluous object creations. (I have not done any benchmarks.)

@littledan
Copy link
Member

I don't know how to judge the performance issues here. Given that this is a performance-motivated feature in the first place (otherwise you could just call Temporal.<type>.from several types), I'm not sure if we can immediately discard the allocation cost.

@pipobscure
Copy link
Collaborator

I think getters are probably the way to implement in a polyfill. The inly question I have is whether we need to specify this or whether it’s better to leave this to implementers.

@gibson042
Copy link
Collaborator

It is always necessary to specify whether properties created by built-in functions are accessors or data properties.

@sffc
Copy link
Collaborator

sffc commented Nov 13, 2019

Why don't we leave this question open for Stage 3 and get feedback from implementers when they get around to implementing it.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2019

@sffc it's a stage 2 concern, not a stage 3 concern (altho changing it as a result of implementer feedback is fine in stage 3). The proposal has to pick one to be eligible for stage 3, imo.

@pipobscure
Copy link
Collaborator

Ok. In that case let’s go with getters. It makes it easy, and if there is implementer feedback we can change it.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2019

I’d prefer normal data properties, absent compelling evidence to make them getters.

@littledan
Copy link
Member

I do agree with Jordan that we need an initial answer here for Stage 3. It would be good to understand the argument for data properties--could you explain the reasoning why you would prefer it?

@ljharb
Copy link
Member

ljharb commented Nov 15, 2019

It makes no sense to me to make a “mini class” for this use case (altho i agree it would solve the question posed here).

A plain object with data properties makes sense to me; and adding yet another class/type to a crowded proposal seems like overkill. If it’s not going to be a plain object, i think that “parse” should be deferred to a followup proposal.

@gibson042
Copy link
Collaborator

I agree that we shouldn't suddenly break with conventions here.

What conventions are those? https://www.ecma-international.org/ecma-262/10.0/index.html doesn't seem to define very many built-in accessor properties:

  1. {RegExp,Array,%TypedArray%,Map,Set,ArrayBuffer,SharedArrayBuffer,Promise}[Symbol.species]
  2. Symbol.prototype.description
  3. RegExp.prototype.{source,flags,dotAll,global,ignoreCase,multiline,sticky,unicode} (with an interesting distinction of lastIndex as a data property on RegExp instances)
  4. %TypedArray%.prototype.{buffer,byteLength,byteOffset,length,[Symbol.toStringTag]} (with an interesting distinction of BYTES_PER_ELEMENT as a data property on each TypedArray prototype object)
  5. Map.prototype.size
  6. Set.prototype.size
  7. ArrayBuffer.prototype.byteLength
  8. SharedArrayBuffer.prototype.byteLength
  9. DataView.prototype.{buffer,byteLength,byteOffset}
  10. Object.prototype.__proto__
  11. %FunctionPrototype%.{caller,arguments}
  12. ordinary arguments.callee
  13. exotic arguments map (not exposed to code)

Setting aside Symbol.species and special legacy-related use, it seems to me that code-accessible built-in accessor properties are for exposing immutable primitive-valued characteristics of directly-constructible instances. In which case they wouldn't make sense here.

@ryzokuken
Copy link
Member

@gibson042 @ljharb do we have consensus on this?

@littledan
Copy link
Member

If the purpose is ergonomics, then I'd be in favor. But if the goal is performance, I don't know if we can conclude a priori that it will be faster to construct this kind of structure. (Note that we're making lots of other decisions that make it more difficult to implement Temporal with high performance, e.g., Timezone/Calendar subclassing.) Absent some more evidence/data that this kind of use case is an actual performance problem that this sort of API can resolve, I'd lean towards omitting this feature from the initial version of Temporal.

@sffc
Copy link
Collaborator

sffc commented Mar 13, 2020

We discussed this on the champions call today.

It seems useful to make a convenience method returning at least { dateTime, timeZone }, since those are the two explicit components of the ISO string. However, it's less clear that we need to also need to include date, time, absolute, etc., since those can be quickly extracted from dateTime and timeZone.

I also wanted to suggest naming this method Temporal.parseISOString to emphasize that this function is about ISO strings, and not any other weird formats like Date.parse supports.

@ljharb
Copy link
Member

ljharb commented Mar 13, 2020

Is there no type that’s a combination of DateTime and TimeZone?

@sffc
Copy link
Collaborator

sffc commented Mar 13, 2020

Is there no type that’s a combination of DateTime and TimeZone?

We have DateTime and Absolute, and you use a TimeZone to transition between them. We previously had ZonedDateTime, but that's been gone for a long time. It was removed in #220

@ljharb
Copy link
Member

ljharb commented Mar 13, 2020

Seems like this is the perfect use case for it, rather than an object with two things in it ¯\_(ツ)_/¯

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2020

If we leave the spec until later, how close is this to being mergeable? I think it's probably doable for our v1 of the polyfill.

@sffc
Copy link
Collaborator

sffc commented May 20, 2020

Discussions about re-adding a combined type (ZonedAbsolute) have been reopened in #569.

@ryzokuken ryzokuken added this to the Stable API milestone May 20, 2020
@ptomato
Copy link
Collaborator

ptomato commented May 28, 2020

Meeting, May 28: We won't be shipping this in the initial polyfill but will revisit it for Stage 3 (including a naming concern raised by @justingrant)

@ptomato ptomato modified the milestones: Stable API, Stage 3 May 28, 2020
@ptomato ptomato modified the milestones: Stage 3, Stable API Jun 4, 2020
@justingrant
Copy link
Collaborator

Meeting 2020-09-11: We do not plan to ship this parsing method because its use cases are almost entirely covered by LocalDateTime.from.

@pmoleri
Copy link

pmoleri commented Mar 30, 2021

What I'm interested in, is a convenience method similar to the one proposed here to help me differentiate unknown date-time strings.

e.g.
2020-10-23 -> PlainDate
2020-10-23T10:50 -> PlainDateTime
2020-10-23T10:50:00Z -> Instant
...

So, I can use it like:

const temp = Temporal.parse(dateString);
if (temp instanceof Temporal.PlainDate) {
   ...
} else if (temp instanceof Temporal.PlainDateTime) {
   ...
} else {
   ...
}

or

const temp = Temporal.parse(dateString);
if (temp.isPlainDate) {
   temp.value // -> PlainDate
} else if (temp.isPlanDateTime) {
   temp.value  // -> PlainDateTime
} else {
   ...
}

Does the API provide any other way of doing something like this?

@ptomato
Copy link
Collaborator

ptomato commented Mar 30, 2021

You can do something like this:

let temp;
try {
  temp = Temporal.ZonedDateTime.from(string);
} catch {
  try {
    temp = Temporal.PlainDateTime.from(string);
  } catch {
    ...
  }
}
if (temp instanceof Temporal.ZonedDateTime) {
  ...
} else {
  ...
}

Note that valid PlainDate strings are also valid PlainDateTime strings, and in other cases the notion of determining a string parsing uniquely into a type into is ill-defined (e.g. Temporal.PlainYearMonth.from('202005') vs. Temporal.PlainTime.from('202005').)

@pmoleri
Copy link

pmoleri commented Mar 30, 2021

Yes. Although, those try/catch don't look too appealing to me.

About ill-defined parsing, it's just a matter of well-define it, right?

> Temporal.PlainYearMonth.from('202005').toString()
"2020-05"
> Temporal.PlainTime.from('202005').toString()
"20:20:05"

Meaning that Temporal.parse() could be defined as being able to parse selected formats that are unambiguous.

@ptomato
Copy link
Collaborator

ptomato commented Apr 19, 2021

Parsing a string into a data type that you don't know in advance is not in alignment with Temporal's goal of strong typing. It can be done, certainly, but does not necessarily need to be done inside the JS specification, and I'd argue that it should not be, since it's not clear that there is a universal way to resolve these ambiguities that will be correct for everyone's use cases.

@pmoleri
Copy link

pmoleri commented Apr 19, 2021

I see. Perhaps I was too loose when I mentioned "differentiate unknown date-time strings". I was always thinking in unknown ISO 8601 variants.

Just like, various Temporal .toString() methods always return ISO 8601, Temporal.parse(validISODateTimeString) would always return a valid Temporal object. So that anyTemporal.equals(Temporal.parse(anyTemporal.toString())). Although, I don't know ISO 8601 spec in such a depth to be sure that there are no ambiguos strings, I was hoping not.

The last section in the docs is about String Persistence. Wouldn't it be nice if it works both ways? I would have a method to persist and another to parse?

@ptomato
Copy link
Collaborator

ptomato commented Apr 28, 2021

You do have a method to parse: it's Temporal.X.from(), where X is the type that you want to parse the string into.

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 enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants