typed-ember / glint

TypeScript powered tooling for Glimmer templates
https://typed-ember.gitbook.io/glint
MIT License
108 stars 50 forks source link

Add `Date` to ContentValue #708

Open NullVoxPopuli opened 3 months ago

NullVoxPopuli commented 3 months ago
per discussion, toString won't happen Anything with `{ toString(): string }` is renderable, including Date, boolean, etc. We should reflect the reality of what is renderable in the `ContentValue` type. It already has `{ toHTML(): string }` support, which is great, but `{ toString(): string }` should cover most other cases. ----- Old Description:

Dates are renderable.

otherwise, you gotta cast your value to something like x as Date & { toHtml(): string } which is silly, and relies on secret knowledge of how SafeString works.

runspired commented 3 months ago

I'm not sure we should type either API unless they are fixed to be reactive, see https://github.com/glimmerjs/glimmer-vm/issues/1578

I think this because without these things being reactive, types may steer folks to greater usage and a lot of silent, difficult to debug bugs around accidental memoization.

dfreeman commented 3 months ago

Anything that inherits from Object (which is most things) has a toString method. However, [object Type] is rarely something people intentionally emit, which is why ContentValue exists.

chancancode commented 3 months ago

Philosophically, I would categorize this as div.innerText/innerHTML = new Date() "works" in JavaScript but errors in TypeScript, and as I mentioned in https://github.com/glimmerjs/glimmer-vm/issues/1578, I take the position that toHTML() is more of an implementation detail than a public API, and if we had symbols way back then we may have chosen to use it, and ultimately the feature will probably go away. So I am not sure this is a direction we want to lean into.

NullVoxPopuli commented 3 months ago

I'll revert back to just Date then..

NullVoxPopuli commented 3 months ago

changed pushed! :tada:

chancancode commented 3 months ago

I am 🤷🏼‍♂️ on adding Date specifically, but I would like to point out that:

otherwise, you gotta cast your value to something like x as Date & { toHtml(): string } which is silly, and relies on secret knowledge of how SafeString works.

No I don't think that's what any of us would like to see or is seriously proposing.

What you would need to do is to cast it to a string explicitly, just like you would when calling any function in TypeScript that specifically requires a string. So really you just have to do the equivalent of String(date) or date.toString() in the template, and that applies to anything that isn't covered here. And it would be the same thing you would have to do for .innerHTML = ... etc and isn't all that uncommon in writing TS code.

What is slightly unfortunate is that there is no convenient method call syntax in the template (which is a separate thing we can and probably should add). It's also a bit of a dance to grab the global String function in the template.

const { String } = globalThis;

<template>
  {{String ...}}
</template>

or... (doesn't work today but I think we agreed it should)

<template>
  {{globalThis.String ...}}
</template>

Not that much of a dance, but it is a dance.

However, you probably really want to have a dedicated helper for this in your app anyway, specifically:

function String(value: unknown): string {
  let string = String(value);
  assert("not like that!", string !== "[object Object]");
  // and any other checks you may want
  return string;
}

..in which case it's just another helper you would import just like any others. And it does auto-track.

I would also say that, of all the things we can choose to cover here, the utility of having Date on the list feels a bit dubious. Having, say, BigInt on there would probably be more useful, but I am not sure when was the last time I actually wanted to render the toString() value of a Date to an user ("Wed Mar 20 2024 14:13:31 GMT-0700 (Pacific Daylight Time)" is quite a mouthful). You would typically want to format that with something like moment (or Intl or whatever people use these days), and even with just methods on a date there is toDateString(), toISOString(), ... so it's not really clear defaulting to Date.prototype.toString() without error/explicit programmer action is a good default.

Same goes for a few of the other things that have a non-trivial toString() but still seems dubious to render to the end user: such as Function.prototype.toString() (do you really want to print your probably-minified function source code...?) and Error.prototype.toString() (are you sure you don't want just .message instead? is it even a good idea to render the raw error message to an end user?). I suppose you could come up with cases where that's what you want, but in many more other cases, you probably accidentally got yourself a class, thinking it is something else or thinking it would render as a component, but it didn't. All the reasons from a type safety perspective, I think requiring you to confirm that's what you really want is a pretty reasonable compromise, and again, align with just about every TypeScript APIs that has this characteristic.

I think it is important to highlight we aren't really talking about a missing capability here. You can definitely render the toString() value in a template if that's what you want, and there are definitely ways you can convince Glint to let you do that, all without doing anything all that weird or usual like x as Date & { toHtml(): string }. What we are really discussing is that if this should be allowed/silenced by default, and I think there are plenty good arguments for why not.

Ultimately though, I suppose it's more a matter of taste/philosophy/consistency and I'd defer to @dfreeman for that. Notably, TypeScript isn't terribly consistent:

dfreeman commented 3 months ago

What we are really discussing is that if this should be allowed/silenced by default, and I think there are plenty good arguments for why not.

I'm inclined to agree. While the default toString() representation of a Date is human readable, it's rare that that's the formatting you'd actually want to expose to the user.

Same goes for a few of the other things that have a non-trivial toString() but still seems dubious to render to the end user: such as Function.prototype.toString() (do you really want to print your probably-minified function source code...?) and Error.prototype.toString() (are you sure you don't want just .message instead? is it even a good idea to render the raw error message to an end user?)

100%, exactly.

console.log() takes unknown[]

In the context of this discussion, I think it's worth noting that console.log generally doesn't coerce values to plain strings—it will generally provide a rich rendering of complex objects, so unknown[] is appropriate.

Ultimately though, I suppose it's more a matter of taste/philosophy/consistency and I'd defer to @dfreeman for that.

I'm not the keeper of the keys on this project at this point, but to the extent my opinion still holds weight I agree that Date isn't consistent with the design goal of the ContentValue constraint.