-
Notifications
You must be signed in to change notification settings - Fork 156
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
Recommended patterns for detecting and cloning any Temporal object #1841
Comments
I would definitely hope there's something more reliable than |
I don't think there's a single slot that's present in all Temporal types, but other champions can correct me if I'm wrong. As an aside, using try/catch for this purpose can be frustrating for developers using browser or IDE debuggers because it complicates "break on caught exceptions" workflows for frameworks like React where exceptions are swallowed by the framework, I assume to make non-debugger error handling have better DX? Anyway, debuggers have different ways to defeat this (e.g. by letting users label some code that should be ignored when exceptions are thrown there) but AFAIK there's no automated way that debuggers can hide these "expected exceptions" from users without manual intervention. It'd be great to not make this problem worse if possible. |
Failing that, I'd expect that such a thing exists for each Temporal type (more to the point, this part is an axiom of the language that this is true, so it's a serious normative bug if it's not). "break on caught exceptions" is already unrealistic, and the Way that brand checks are done in JS is, sadly, try/catch - see https://npmjs.com/~inspect-js. |
@ljharb Which part are you saying is an axiom of the language? That a dedicated method exists for brand-checking, or just that it is possible? Currently it is possible, every method on a Temporal object does a brand-check, so you can do something like this const ReflectApply = Reflect.apply;
const TemporalInstantPrototypeToString = Temporal.Instant.prototype.toString;
// ...etc.
function isTemporalInstant(value) {
try {
ReflectApply(TemporalInstantPrototypeToString, value, []);
} catch (e) {
return false;
}
return true;
} @justingrant I think what you are asking for is something different? That is, a Temporal "brand-check family" and a way to check if an object's brand is part of that family? |
@ptomato i'm told today that "invariant" is a better term to use than "axiom" :-) I'm saying it's an invariant that brand checking be possible. The only exception - due to oversight - is Error, and the stack traces proposal would fill that gap. I agree that it seems like @justingrant wants something that can check the bag of Temporal types all at once instead of individually hardcoding each brand check (which is what i'd otherwise do in the various https://npmjs.com/~inspect-js packages i will be making) |
I have been doing some more reading on invariants and I think it'd be useful for future discussions to update https://github.com/codehag/documenting-invariants/blob/master/abandoned_invariants.md#objectprototypetostringcall-as-a-brand to reflect this. It doesn't seem correct that that's listed under "abandoned invariants", if it was accidentally broken and it should still hold for any future types? |
@ptomato It was intentionally broken, and the committee decided not to withdraw |
I think we are in agreement! I'm trying to say, the following text from that link:
which sounds like the same thing you are saying, should be documented clearly in that repo, and not listed as part of "abandoned invariants" as it is now. (Even if the rest of the text about Object.prototype.toString no longer holds) |
Ah, totally agree then :-) |
@ljharb Would you like to take a crack at it? It sounds like you know all the context for this invariant. @justingrant What do you think would be the best resolution for this issue? A cookbook recipe and/or a discussion topic for temporal-v2? |
Cookbook recipe sounds good. |
While testing Temporal in one of my apps, I ran into a failure that turned out to be caused by the
clone
library being unable to deep-clone objects that included Temporal instances as properties. Looking at the source ofclone
, it's clear why cloning won't work for Temporal instances. (Nor, I assume, with many other types like those that use private fields, WeakMaps, etc.)There are a lot of similar libraries that do deep cloning. I assume that all of these libraries will need to be updated to know about Temporal, just like they needed to be updated to recognize other ES built-in types like Map and Set.
I assume it would be helpful for us to provide a recommended implementation of "Is this a Temporal instance?" and "clone this Temporal instance" code. I built a simplistic draft of these operations below. What am I missing?
In particular, is it better to identify "is this a Temporal instance" by comparing the
constructor
property to Temporal constructors? Or is it better to be more loose, like this?Here's a starting point:
The text was updated successfully, but these errors were encountered: