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

Accessing this inside a template tag in a test context #61

Open
evoactivity opened this issue Sep 4, 2024 · 11 comments
Open

Accessing this inside a template tag in a test context #61

evoactivity opened this issue Sep 4, 2024 · 11 comments

Comments

@evoactivity
Copy link

evoactivity commented Sep 4, 2024

In a class backed component a template tag has access to implicit this

class FooBar extends Component {
  foo = 'bar';
  <template>{{this.foo}}</template> // <-- totally fine to access this here
}

In a test context, a template tag does not have access to implicit this

hooks.beforeEach(function () {
  class State {
    @tracked whatever;
  }
  this.instance = new State();
})

test('...', async function () {
  let context = this.instance; // <-- I can access this here
  await render(<template>
     {{context.whatever}} <-- this is ok
     {{this.instance.whatever}} <-- why can't I access this here?
  </template>);
});

This difference in behavior can be confusing.

This has been talked about before on this PR #40

@elwayman02
Copy link

Relevant context from @NullVoxPopuli:

This syntax:

const whatever = '...';

<template>

</template>

is template-only syntax.

Which uses this component manager, and has no getContext method https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/runtime/lib/component/template-only.ts#L32

like the @glimmer/component does: https://github.com/glimmerjs/glimmer.js/blob/v1.1.2/packages/%40glimmer/component/addon/-private/base-component-manager.ts#L57

@elwayman02
Copy link

The key here, in my mind, is that it would be nice if it were possible to pass a context to <template> as a general API, one application of which would be to send a TestContext to the template so we can invoke this.whatever the same way we would in the hbs format, allowing developers to utilize the established patterns of setting up shared test context in QUnit's beforeEach hook without having to do strange workarounds like variables scoped to the module function, which would be prone to state leakage especially if we wanted to parallelize individual tests in the future.

@elwayman02
Copy link

Another potentially confusing thing here is that I believe the context setting for <template> only happens if it's a component class, not all classes. So, while it seems unlikely someone might try to do this (the testing pattern is far more important and useful), it does present another confusing inconsistency in the developer experience:

class FooBar {
  foo = 'bar';
  <template>{{this.foo}}</template> // <-- cannot access this here
}

class FooBar extends Component {
  foo = 'bar';
  <template>{{this.foo}}</template> // <-- totally fine to access this here
}

@NullVoxPopuli
Copy link
Contributor

I think one way forward, would be three phase:

  • add support for glimmer-vm for this to be set externally, at the place of usage -- with getContext from the component managers taking precedence
  • update babel-plugin-ember-template-compilation to use that new thing
  • update managers to remove the getContext for @glimmer/component and @ember/component (the only supported classes for <template></template> right now so that the already passed in context would be interpreted as the this

Of note, something that would still work through this process, is the XState component manager:

const Toggle = createMachine(...);

<template>
  <Toggle as |blah|>
  </Toggle>
</template>

because you can't extend it in any way that would give you access to a this anyway.

This probably requires an RFC

@ef4
Copy link
Contributor

ef4 commented Oct 11, 2024

I think this is just a bug and doesn't need an RFC.

In expression position, there's no reason this can't be accessed lexically just like any other local. The bug is in two places.

  • This repo needs to relax the syntactic limitation of the scope bag (right now I don't think allows string keys, and you need a string key to represent this in an object literal since its otherwise a keyword).
  • content-tag needs to put this in the scope bag when it's been used and we're in Expression position (this should not be done in class position)

@chancancode
Copy link
Member

chancancode commented Oct 11, 2024

I don't really have an opinion on whether it needs an RFC, personally I also support/want this feature. However implementation wise it would require a bit more coordination than just what was laid out above, since this syntactically/semantically significant even in the glimmer pipeline.

I don't think just adding it to the locals array when calling the compiler would make it work, and arguably if it does work it's a bug just as much as if it allowed @foo as a local.

It isn't tremendously complicated to plumb that through on the Glimmer side, but that needs to happen. Alternatively, the variable can be rewritten a into a different name (e.g. __this__). Functionally, that should work, though perhaps with other unintended consequences when it comes to AST transforms and source maps.

At a high-level though, I agree with @ef4's general thinking that this is more appropriately modeled as a local variable capture in expression positions (i.e. arrow functions) rather than messing with the runtime component manger's getContext hook etc

@chancancode
Copy link
Member

We also needed to do something similar to support private fields (don't remember if that has been RFC'ed either, but I think everyone thinks/assumed that should work)

@ef4
Copy link
Contributor

ef4 commented Oct 11, 2024

Poking more, this also requires a fix in ember-source or glimmer-vm. The template compiler itself is special-casing this and ignores this in the lexical scope bag.

@ef4
Copy link
Contributor

ef4 commented Oct 11, 2024

(I commented before I saw @chancancode's updates, we are in alignment here.)

@wycats
Copy link
Member

wycats commented Oct 18, 2024

Is the idea that we'd key ambient this capture on whether or not the component option was passed to template?

class FooComponent {
  static {
    template("{{this.foo}}", {
      eval() { return eval(arguments[0]),
      // are we saying that this flag makes the template a "constructor-style"
      // template, which *does not* capture `this`?
      component: this
    })
  }
 
  // but this template (because it doesn't pass the `component` option,
  // is an "arrow-style" template, which *does* capture `this`?
  t = template("{{this.foo}}", { eval() { return eval(arguments[0]) })

}

This seems perfectly sensible, and I think it works. That said, keying this behavior on whether component is passed as an option feels subtle. Given the significance of the difference, do we perhaps want a different entry point for the declarative class-element form vs. the expression form?

@ef4
Copy link
Contributor

ef4 commented Oct 18, 2024

Is the idea that we'd key ambient this capture on whether or not the component option was passed to template?

Yes.

Given the significance of the difference, do we perhaps want a different entry point for the declarative class-element form vs. the expression form?

That would have been good feedback for RFC 931. But all this stuff shipped quite a long time ago, and I don't think this rises to the level of justifying ecosystem migration.

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

6 participants