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

JS Representation of Template Tag #931

Merged
merged 14 commits into from
Jun 9, 2023
Merged

JS Representation of Template Tag #931

merged 14 commits into from
Jun 9, 2023

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented May 23, 2023

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label May 23, 2023
@ef4 ef4 added S-Exploring In the Exploring RFC Stage and removed S-Exploring In the Exploring RFC Stage labels May 31, 2023
@ef4 ef4 marked this pull request as ready for review May 31, 2023 16:02
### Type Signature

```ts
import { ComponentLike } from '@glimmer/template';
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.


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

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.

Comment on lines 258 to 259
params?: ExplicitParams | ImplicitParams,
backingClass: C;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

}

interface ExplicitParams extends BaseParams {
scope?: (instance: object) => Record<string, unknown>;
Copy link
Contributor

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:

  1. Note: the union will allow you to pass both eval and scope. playground. We have a choice to make here:

    I think either case is fine, since this is a low-level API which is mostly emitted by tools.

  2. 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:

  1. It moves the class argument before the params to avoid the issue with optional params preceding required params.
  2. It introduces a StrictUnion type to force excess property checking to do what we would expect so you cannot pass both scope and eval.
  3. It updates the return type from eval to be unknown instead of any. While eval() itself returns any, this means our own code is stricter and safer here.
  4. It passes no argument at all to the scope in the case of template-only components, and passes the instance type scope in the case of class-backed components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

@chriskrycho chriskrycho left a 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!

@wagenet wagenet added S-Exploring In the Exploring RFC Stage Final Comment Period and removed S-Proposed In the Proposed Stage labels Jun 2, 2023
@wagenet wagenet requested review from a team June 2, 2023 18:46
@wagenet wagenet requested review from a team June 2, 2023 18:46
@wagenet
Copy link
Member

wagenet commented Jun 2, 2023

Just moved to FCP!

@kaermorchen
Copy link

kaermorchen commented Jun 4, 2023

Hi all!

How will private fields work in children components?
For example:

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?

@ef4
Copy link
Contributor Author

ef4 commented Jun 4, 2023

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.

@flashios09
Copy link

Hi,
does this mean we will be able to use valid js/ts without needing the gjs/gts extension ?
the template() function could be used instead of the <template /> angle brackets ?

@chriskrycho
Copy link
Contributor

Yes… but that’s already true of the compilation format this will supersede, too. It’s not particularly ergonomic, though:

  • Every single thing you reference in your template will have to be manually named in the scope object.
  • There is not syntax highlighting for it and it’s unlikely anyone will develop it.
  • We will not be providing Glint support for it (since it is a low-level format not intended for normal authoring).
  • There will not be any Prettier or ESLint or ember-template-lint support for it.

…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!

@lifeart
Copy link

lifeart commented Jun 5, 2023

Some things to highlight from https://twitter.com/nullvoxpopuli/status/1665462066960912387

  • At the moment there is no real-life use-case for private property access inside templates.
  • We still not support [string properties like this], and such cases should be accessed using get helper.
  • If property is defined via symbol, we also have to use get helper and symbol reference.
  • We already use get helper for array by-index access.

So, private property access may be also available only via get helper (if needed). But I do see lack of real-life use-cases for private properties in components. It may be absolutely valid inside libs (glimmer/starbeam), services, business logic classes and cetra, but for view layer it's looks really strange.

@wagenet wagenet dismissed chriskrycho’s stale review June 9, 2023 18:28

Concerns have been addressed.

@wagenet wagenet merged commit 3bce77c into master Jun 9, 2023
@delete-merged-branch delete-merged-branch bot deleted the template-compiler-api branch June 9, 2023 18:29
@ef4 ef4 mentioned this pull request Jun 23, 2023
chancancode added a commit that referenced this pull request Oct 24, 2023
This brings back some details about runtime template compilation
that got lost from #813
chancancode added a commit that referenced this pull request Oct 24, 2023
This brings back some details about runtime template compilation
that got lost from #813
ef4 added a commit that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants