-
Notifications
You must be signed in to change notification settings - Fork 157
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
Comments
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 |
|
@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. |
If you have the Absolute, can you not cheaply derive the others from it? |
@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. |
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? |
Can we reach consensus on if this needs to be done? |
Resolution: Yes
|
Beautiful :-) |
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. |
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}) |
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? |
What does the getter do for missing parts? If it just returns undefined then ok, if it throws that would be bad. |
I was imagining it would return undefined. |
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? |
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.) |
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 |
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. |
It is always necessary to specify whether properties created by built-in functions are accessors or data properties. |
Why don't we leave this question open for Stage 3 and get feedback from implementers when they get around to implementing it. |
@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. |
Ok. In that case let’s go with getters. It makes it easy, and if there is implementer feedback we can change it. |
I’d prefer normal data properties, absent compelling evidence to make them getters. |
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? |
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. |
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:
Setting aside |
@gibson042 @ljharb do we have consensus on this? |
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. |
We discussed this on the champions call today. It seems useful to make a convenience method returning at least I also wanted to suggest naming this method |
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 |
Seems like this is the perfect use case for it, rather than an object with two things in it ¯\_(ツ)_/¯ |
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. |
Discussions about re-adding a combined type (ZonedAbsolute) have been reopened in #569. |
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) |
Meeting 2020-09-11: We do not plan to ship this parsing method because its use cases are almost entirely covered by |
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. So, I can use it like:
or
Does the API provide any other way of doing something like this? |
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. |
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?
Meaning that |
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. |
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 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? |
You do have a method to parse: it's |
We should consider a way to parse a string once, and output multiple objects. Like this:
returns an object with properties:
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:
I kinda hate the string array so ideas welcome there.
@mj1856 for reference.
The text was updated successfully, but these errors were encountered: