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

support { toString(): string } and { toHTML(): string } as reactive outputs #1578

Open
NullVoxPopuli opened this issue Mar 19, 2024 · 8 comments

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Mar 19, 2024

I don't see supporting either of these things as breaking, as today, you got [object Object] or similar when rendering "things with methods".

What we need to make sure though, toString(), and toHTML() are reactive.

Could replace:

AppendHTML: 43 satisfies VmAppendHTML,
AppendSafeHTML: 44 satisfies VmAppendSafeHTML,
AppendDocumentFragment: 45 satisfies VmAppendDocumentFragment,
AppendNode: 46 satisfies VmAppendNode,
AppendText: 47 satisfies VmAppendText,

@chancancode
Copy link
Contributor

What is the use case?

@NullVoxPopuli
Copy link
Contributor Author

@runspired
Copy link
Contributor

one of the key things this allows is greater shareability and usability of boxed values (like translations). Today, there's no path for folks to use translations in module scope or in functions in module scope which leads to a lot of very odd patterns trying to map a mostly static concept to something app instance based.

Beyond intl, at AuditBoard we've got a lot of use cases for boxed values around concepts like linkify: linkify is ostensibly an html value on a model coming from the API but really its an object with a bunch of knowledge of various things that can also be rendered as a link in a template context.

EmberData itself has a lot of use for boxed values as well, making things like a rework of transforms capable of providing a renderable value while also containing additional meta information for usage in JS form.

@runspired
Copy link
Contributor

Also I'd note that I personally consider this change to be use-the-platform / feature-fill / fixing of the mental mode. It's generally presumed that if a value is rendered from a template, any tracked state that it accesses would result in a re-render when necessary. Glimmer today sort of accidentally memoizes a lot of things by only accessing these things from outside of the tracking context.

@chancancode
Copy link
Contributor

Not obviously a good idea to me:

  • toHTML() is better thought of as a private implementation detail (of SafeString/htmlSafe()) in Ember, instead of a public API in and of itself. As discussed in Allow to render TrustedHTML objects similar to SafeString #1567, it's quite possible that the ultimate fate of the feature in the long term is to drop it in favor of the platform primitive, those are definitely meant to be viewed as immutable wrapper objects around the underlying string, which is itself immutable.

  • toString() is also somewhere in between; it's better to think of it as we expect the append positions to be a primitive scalar value but if we don't get one we have to do something, and because historically our error infrastructure is not good and historically we try to be keep things pretty weakly typed, we stringify the value instead of erroring, but I think it's better to think of it as document.createElement({ toString() { ... } })/new Date({ valueOf() { ... } }) "works", but maybe not necessarily the best idea ever.

Specifically for the proposed change, – things would have to bottom out somewhere and you have to be able to expect a "scalar" value somewhere. Even in JavaScript, you generally can't e.g. return another another "string-like" from toString().

It's not that it's a hard feature to add, but I am not sure it's worth the cost/tradeoffs. Right now the code gets to make the assumption that when you get to the append/"value" position it bottoms out, and essentially this would force us to give that up and wrap everything in yet another tracking frame just in case, and I don't think it's worth it.

Glint also isn't going to be happy with this. You can say {{...}} just takes an unknown or any, but that comes as the expense of not catching a lot of bugs.

If we have a method call syntax then imo it's not a big deal to have to call foo.toString() yourself (but at that point, since you control the class, it's also fine to just do {{foo.value}} or something and make it a getter.

There are a lot of things we could do and a lot of things we can make more dynamic. We can have even an "value manager" to decide how to render any {{...}}. Heck, we can even wrap a tracking frame around getManager() and allow that to be reactive. But everything you add and make more dynamic has a cost and you'd need to consider that and make sure the feature pays for itself, especially when you are also so worried about performance.

@runspired
Copy link
Contributor

I'm on the other side in that I think this is a blatantly obviously good thing to do 😅

Let's focus the convo on toString as by-and-large it should be the case that toHTML could be eliminated in favor of TrustedHTML, its polyfill or similar should that spec fail to be adopted. Though first a sidenote: TrustedHTML also requires us to support toString in this same manner.

Supporting the platform here would by-far simplify the internals of glimmer from what I can tell poking around. And you've already answered your own question with must bottom out somewhere. Yes ... toString does always bottom out because the platform ensures it does when invoked from String like we do (and should continue to do). We don't actually need a separate tracking frame, we just need the existing String call to be moved within the existing tracking frame.

image

Taking this approach is also drastically simpler than adding layers of complexity and additional concepts like a value manager, and presents nicer DX than forcing a helper invoke in templates or (much worse) forcing a property like value for literally every value in every app in every template like we would need to do today to fix this problem.

What it doesn't do is make glint all that happy, because typescript doesn't have a good way to know if this is a meaningful toString or not. And to that end, outside of typed templates maybe understanding a few primitives like Date or TrustedHTML, that's where a symbol probably should be encouraged. But the burden should be on folks using typescript to teach typescript how to understand boxed values like this, vs glimmer breaking platform expectations in order to appease typescript.

@chancancode
Copy link
Contributor

Though first a sidenote: TrustedHTML also requires us to support toString in this same manner.

It doesn't actually, as per discussion in #1567, we need to specifically not call String(...) on trusted values and pass it through to the DOM APIs in order for it to not trigger the CSP warnings (which is the whole reason why changes are required, otherwise it already "works" since we stringfy everything).

I don't think that's super relevant other than that, if your plan involves writing toHTML() that consumes tracked state, and say we made that work today, you may eventually not have a transition path at all. And to that end, if it feels "natural" that toString() works that way, then it would be an asymmetry.

We don't actually need a separate tracking frame, we just need the existing String call to be moved within the existing tracking frame

There is no "existing" tracking frame here in this context, it will be a new tracking frame inserted just to make this work. Specifically when we are calling String(value) it's very late in the game right before inserting the value into the DOM (textNode.value, setAttribute, and what have you). The value probably came from reifying a reference, but the reference could represent a value that came from anywhere, there may or may not be a notional tracking frame inside it, depending on how that value came into existence in the first place (it could be a property path, it could be the result of computing a helper, it could be a string constant from 5 components above, it could be a constant from the "scope bag", ...), but even when there is one, because we are operating on the concept of a reference generically at this spot, you can't just piggy back on the existing tracking frame.

Currently we just pull the reference and check/coerce the resulting value as needed, but to make this work we would have to specifically add/wrap a tracking frame around all of these coercion points just in case. And to make this work consistently it would have to be the same in all of these sink positions (properties, attributes, etc) in ways, and in a few of these spots that can greatly complicate things on the off chance that it did end up changing.


And, to re-iterate what I said in typed-ember/glint#708 (comment), we are not exactly talking about a missing capability here. You can definitely experiment and use boxed values and what not, we are just talking about whether we should always automatically/transparently (and reactively) unbox. I further agree it definitely wouldn't have be surprising if it did work that way, and if it's free to do I would probably say why not. But since we are talking about "always/transparently", it comes down the the tradeoff of the extra cost vs how common this really is.

The alternative is that you add a helper like this in the app:

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;
}

But since this is a relatively rare occurrence, and that, as you said, a symbol is probably a better/more deliberate protocol/opt-in, you could also just do that:

const VALUE = Symbol("BOXED VALUE");

export interface BoxedValue {
  [VALUE](): string; // | undefined or whatever your needs are
}

export function unbox(boxed: BoxedValue): string {
  return boxed[VALUE]();
}

Then {{unbox ...}} would work just fine.

If you think about it, you could, hypothetically, AST transform all your {{...}} in your app to {{unbox ...}} transparently and get pretty much the same result, it actually wouldn't be that different than what the code would have to do if we add it to core.

But if that gives you a slight pause... that's roughly where I am coming from as well.

@runspired
Copy link
Contributor

Currently we just pull the reference and check/coerce the resulting value as needed, but to make this work we would have to specifically add/wrap a tracking frame around all of these coercion points just in case. And to make this work consistently it would have to be the same in all of these sink positions (properties, attributes, etc) in ways, and in a few of these spots that can greatly complicate things on the off chance that it did end up changing.

You are seeing my point without seeing my point 🙈

We reduce complexity by having glimmer no longer do this coercion, instead the reference lookup becomes responsible for it. That lookup is also what starts a tracking frame, and so it doing this same check within itself fixes the autotracking problem pretty elegantly. We don't need a new tracking frame at all, and we simplify the branching of the code.

If you think about it, you could, hypothetically, AST transform all your {{...}} in your app to {{unbox ...}} transparently and get pretty much the same result, it actually wouldn't be that different than what the code would have to do if we add it to core.

Given that nearly 100% of values in templates today are secretly boxed values, moving to an explicit unboxed API seems dangerously bad to DX.

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

No branches or pull requests

3 participants