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

Add Date to ContentValue #708

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Date to ContentValue #708

wants to merge 5 commits into from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Mar 12, 2024

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.

@NullVoxPopuli NullVoxPopuli changed the title Add Date to ContentValue Add { toString(): string } to ContentValue Mar 19, 2024
@runspired
Copy link

I'm not sure we should type either API unless they are fixed to be reactive, see glimmerjs/glimmer-vm#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
Copy link
Member

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
Copy link

Philosophically, I would categorize this as div.innerText/innerHTML = new Date() "works" in JavaScript but errors in TypeScript, and as I mentioned in glimmerjs/glimmer-vm#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
Copy link
Contributor Author

I'll revert back to just Date then..

@NullVoxPopuli NullVoxPopuli changed the title Add { toString(): string } to ContentValue Add Date to ContentValue Mar 20, 2024
@NullVoxPopuli
Copy link
Contributor Author

changed pushed! 🎉

@chancancode
Copy link

chancancode commented Mar 20, 2024

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:

  • console.log() takes unknown[]
  • ${ anything } works but type-aware @typescript-eslint rules prevents it (and String(...) is a common idiom for that)
  • innerHTML and pretty much anything else does require string (and String(...) is a common idiom for that)

@dfreeman
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants