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

implementing rfc931 #21

Merged
merged 11 commits into from
Jul 7, 2023
Merged

implementing rfc931 #21

merged 11 commits into from
Jul 7, 2023

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented May 30, 2023

Edited to add: the call signature in the RFC has changed, this PR needs updating to that new call signature.

This implements emberjs/rfcs#931, which defines a stable low-level representation for template tags.

When using targetFormat: 'hbs' it acts as a polyfill for the ahead-of-time compiler as described in the RFC, using only stable public APIs. Assuming the RFC is approved and implemented, this implementation could change so it leaves template() alone, rather than replacing it with precompileTemplate and setComponentTemplate.

When using targetFormat: 'wire' it's not really a polyfill, since this package is the definitive implementation.

There are skipped tests to cover cases that are important but will depend on glimmerjs/glimmer-vm#1421

  • targetFormat: 'wire' with explicit form
  • targetFormat: 'hbs' with explicit form
  • targetFormat: 'wire' with implicit form
  • targetFormat: 'hbs' with implicit form
  • end-to-end test with ef4/content-tag

@ef4 ef4 marked this pull request as draft May 30, 2023 15:18
Copy link
Contributor

@achambers achambers left a comment

Choose a reason for hiding this comment

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

Had a crack at reviewing this against the RFC and it looks good to me. Left 2 comments/questions there for consideration. But overall, LGTM

import templateOnly from "@ember/component/template-only";
import { setComponentTemplate } from "@ember/component";
import { precompileTemplate } from "@ember/template-compilation";
let hasBlock = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ef4 Pretty sure this shouldn't here. ✂️

import HelloWorld from 'somewhere';
export default class {
static {
template('<HelloWorld @color={{red}} />', { component: this, scope: () => ({ HelloWorld }) });
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the RFC to reconcile this implementation and it got me wondering, is there value in a test that shows using the instance arg that's passed in to the scope function, say, for accessing private fields?

This is the code that got me wondering this:

// in class member position, we can also put private fields in scope
class extends Component {
  static {
    template(
      "<Section @title={{this.title}} @subhead={{this.#secret}} />", 
      {
        component: this,
        scope: (instance) => ({ 
          Section, 
          "#secret": instance.#secret
        }),
      },
    )
  }
}

@ef4 ef4 merged commit b5624a3 into main Jul 7, 2023
@delete-merged-branch delete-merged-branch bot deleted the rfc931 branch July 7, 2023 19:16
@ef4 ef4 added the enhancement New feature or request label Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants