-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
JS Representation of Template Tag #931
Conversation
text/0931-template-compiler-api.md
Outdated
### Type Signature | ||
|
||
```ts | ||
import { ComponentLike } from '@glimmer/template'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intent here to ~upstream the type from @glint/template
, or otherwise to provide it from a new @glimmer/template
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just a typo, that was supposed to say @glint/template
.
But also: is that OK? I understand that a lot of that package calls itself private, but it also seems like the best type we have for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, very good. Yes, that specific import is explicitly public and intended for this sort of usage! The only concern we will want to think about is that it forces a consideration for co-evolution of these packages, but that’s largely already true and we also expect the public API here to be quite stable so that’s not a particularly significant concern.
text/0931-template-compiler-api.md
Outdated
|
||
Bare templates are a historical concept that we'd like to move away from, in order to have fewer things to teach. | ||
|
||
When the optional `backingClass` argument is passed, the return value is that backing class, with the template associated, just like `setComponentTemplate`. When the `backingClass` argument is not provided, it creates and returns a new template-only component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the examples above, I notice that the return type is unused in the case of a class-backed component, so what's the reason to have it be returned there?
import { template } from "@ember/template-compiler";
class Example extends Component {
static {
// ❗️ return value is unused ❗️
template(
"Hello {{message}}",
{
scope: () => ({ message }),
},
this
);
}
}
Note that I am not objecting, just observing and curious what your thoughts here are.
text/0931-template-compiler-api.md
Outdated
params?: ExplicitParams | ImplicitParams, | ||
backingClass: C; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This signature is actually forbidden by TS: you cannot pass a required param after an optional param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also a typo, I should have removed the ?
when I split the definition into the two overrides.
Although your other suggestion to reorder these would also probably be fine too.
text/0931-template-compiler-api.md
Outdated
} | ||
|
||
interface ExplicitParams extends BaseParams { | ||
scope?: (instance: object) => Record<string, unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two notes on this type:
-
Note: the union will allow you to pass both
eval
andscope
. playground. We have a choice to make here:- Do we indulge in some type-level shenanigans to forbid it?
- Or do we just ship a lint rule or some such?
I think either case is fine, since this is a low-level API which is mostly emitted by tools.
-
I think we want the callback signature to get an instance where there is a class, and no argument at all (or, for consistency with the current runtime behavior,
null
?) when there is no class passed?
Combining those two notes, I think our final signature here should look something like the one in this playground, which makes the following changes:
- It moves the class argument before the params to avoid the issue with optional params preceding required params.
- It introduces a
StrictUnion
type to force excess property checking to do what we would expect so you cannot pass bothscope
andeval
. - It updates the return type from
eval
to beunknown
instead ofany
. Whileeval()
itself returnsany
, this means our own code is stricter and safer here. - It passes no argument at all to the
scope
in the case of template-only components, and passes the instance typescope
in the case of class-backed components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hit a snag with the idea to reorder the parameters. When the call arity is two, we need to distinguish the case where the second argument is a parameters bag vs a backing class. And I'd rather not make a lot of assumptions about what's inside a component. Maybe it even happens to have a property named scope, etc.
Perhaps instead we should move the backing class inside the params bag. Only downside I see is that it's slightly more wordy, since it will need a property name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the other thought I had. Otherwise, making the params
argument required in both cases also works—and seems fine from the POV that this is a low-level API primarily for tools and secondarily for authors doing relatively low-level work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall 👍🏼 from me on the API direction; left a detailed note on the types here to make them work the way you want them to!
Just moved to FCP! |
Hi all! How will private fields work in children components? class A extends Component {
name = 'A';
#privateField = 42;
<template>A am {{this.name}} {{this.#privateField}}</template>
}
class B extends A {
name = 'B';
} Will component B render privateField? Can private fields in templates make some sort of non-obvious behavior? |
I think the inheritance case should behave just like typical JavaScript. The template is lexically scoped inside the parent class, so it has access. The child doesn't have access. The child can still use the parent's template, but if the child overrides the template it can only refer to public (or in typescript terms, protected) fields. If you think of the template as similar to a method it would be the same. The method in the parent has access, the child can call the method, but if the child overrides the method the child's implementation can't gain access. |
Hi, |
Yes… but that’s already true of the compilation format this will supersede, too. It’s not particularly ergonomic, though:
…all of which also already apply to the target format which this would supersede. You won’t get any of the benefits of the custom syntax, in other words! |
Some things to highlight from https://twitter.com/nullvoxpopuli/status/1665462066960912387
So, private property access may be also available only via |
This brings back some details about runtime template compilation that got lost from #813
This brings back some details about runtime template compilation that got lost from #813
Rendered