Introduce new test helpers for rendering (and re-rendering) that obviate the need for get and set

[cafreeman]

As part of the larger initiative to support TypeScript in Ember, this PR proposes that we provide a new set of test helpers that removes the need to get and set values on the this values in tests. This change would benefit all Ember developers, but would especially help TypeScript users because of how much it would simplify the type-checking in component tests.

rendered

[chriskrycho]

Thank you, @cafreeman – I’m really excited about this!

[sukima]

What is the difference between await rerender() and await settled() like we do now with this.set()?

[muziejus]

I fully agree that this.set and this.get feel like pre-Octane archaisms.

I wonder about the name of the function, scopedTemplate, however. If I understand correctly, this line from the docs:

await render(hbs`<PrettyColor @name={{this.colorValue}} />`);

Would be replaced with something like:

const args = { colorValue: "red" };
await render(scopedTemplate`<PrettyColor @name={{args.colorValue}} />`);

For me, as a learner, the top line clicks right away because the hbs helper stands in for "Oh, what follows is written in hbs," like with graphql in Gatsby or gql in Apollo. Hence the whole line can be read as "await the render of this bit of hbs."

On the other hand, "await the render of this scoped template" forces me to think about why it's important that the template is scoped, and "because it's scoped, it has access to args you defined above" is somehow too precise. I would prefer something like template by itself, if possible, or componentTemplate. That the template has access to data from the surrounding scope is implicit in the examples, I feel.

[chriskrycho]

@sukima—

What is the difference between await rerender() and await settled() like we do now with this.set()?

The RFC covers this:

In the current testing paradigm, we'd usually just update the values using this.set, which immediately triggers a rerender. In more complex cases, e.g. where there is some async operation involved, we would wait for the state we want to assert against, likely using either settled or waitFor. This works, but has the downside of either waiting for everything to finish (in the case of settled), or waiting for a single thing to finish (in the case of waitFor).

Instead, we propose adding a new rerender function to @ember/test-helpers that exclusively waits on pending render operations, but ignores all other settledness metrics…

---

@muziejus—it is indeed a bit of a longer name! @cafreeman, @rwjblue, and I talked about it a fair bit while @cafreeman was drafting this originally. There’s certainly some room for bike-shedding here, but the name is intentionally designed to communicate the difference with classic hbs while we're in the intermediate period before transitioning to something like the First-Class Component Templates (<template>) proposal (#779). The key bit is that it does have access to local scope, whereas hbs does not. Once we land something like <template>, we will be able to just swap it out (with a trivial codemod!), and then the fact that it’s scoped will be obvious. What we wanted to avoid was any ambiguity about what differentiates this from hbs!¹

Footnotes

1. Well, and we also didn’t want to bias the discussion around the alternatives on First-Class Component Templates #779 by introducing another thing named hbs which already has lexical scope! ↩

[simonihmig]

Looking forward to this! Some questions though:

1. The RFC's title mentions deprecating get/set, while the actual text does not even mention that anymore, not what the deprecation message is, what the deprecation guide should suggest etc. Instead the RFC talks about introducing new testing features. My understanding is we will only deprecate APIs when their replacements are already available, so assume it is really this - adding new testing features that will eventually replace the existing ones, but for now both can happily coexist, right? If so, the RFC title is misleading.
2. It is not entirely clear to me what rerender() does, is it triggering the rerender (that was my first impression when reading that word), or just returning a promise that resolves when rerendering has finished? I assume the latter? So rerendering happens whenever a tracked property is mutated (just like in app code), and instead of calling await rerender(), I could omit that call and instead have our usual await settled() (when I do want to wait for other async effects), right?
3. at least when actually deprecating get/set, that would require quite a massive refactoring effort for all existing code bases, with mostly "mechanical" changes. Have you considered providing a codemod? AFAICT that should be doable, and will likely save a lot of migration effort.

[knownasilya]

@chriskrycho why have an intermediate step between <template> support and the current status quo?

[chriskrycho]

Because it means the two aren’t coupled to each other—not for acceptance of the RFCs or for their implementations. If it happens that #779 gets accepted before this does, then we can revise this in terms of it if we so desire. But even then that may not be desireable because it means you are blocked on all the tooling being done and ready.

[cafreeman]

@simonihmig

1. This is a great point, and you're right that this is not as clear as it could be. I should probably change the title since, while the goal is to EVENTUALLY deprecate, the immediate goal is to just provide the new mechanism that would render get/set unnecessary without actually removing it. I'll update for clarity's sake.
2. " So rerendering happens whenever a tracked property is mutated (just like in app code), and instead of calling await rerender(), I could omit that call and instead have our usual await settled() (when I do want to wait for other async effects), right?" Yes, precisely!
3. We would almost certainly provide a codemod (assuming it proves possible) when the time comes for the actual deprecation.

[NullVoxPopuli]

I have example of what this PR implies along with <template> stuff: https://github.com/GavinJoyce/ember-headlessui/pull/119/files#diff-268d073b63281f34748203ce9d28f671ae6be574d74ea8d0407154b0d3452924R29 (WIP)

🎉

[bertdeblock]

Super minor, but I assume we should also update some blueprints as part of this effort?

• https://github.com/emberjs/ember.js/blob/master/blueprints/component-test/qunit-rfc-232-files/__root__/__testType__/__path__/__test__.ts#L10-L11
• https://github.com/emberjs/ember.js/blob/master/blueprints/helper-test/qunit-rfc-232-files/__root__/__testType__/__collection__/__name__-test.ts#L10-L16

[rwjblue]

Chatted with @cafreeman about this last week, I think we are in a pretty good spot to pick this RFC back up (now that first class <template></template> RFC has landed, things get nicely simplified here).

@cafreeman - I'm gonna move this into "draft" (since we know there are some tweaks that are needed) and you can move it back to ready for review once updated, then I'll try to make sure to get this on the agenda for discussion / forward progress.

[cafreeman]

Now that First-Class Component Templates have landed(🎉 ), I've updated the RFC to use the <template> instead of trying to add a scopedTemplate function. This simplifies things a great deal makes the RFC (and its implementation) quite a bit tidier.

[cafreeman]

@rwjblue Moving this back to "ready for review" now that it's been updated.

[rwjblue]

Thank you!! I'll work to get this on an upcoming core team agenda...

[rwjblue]

We discussed this at the core team meeting on Friday (2022-04-01), and are generally all in favor of moving forward. There are just a few tweaks/changes that we'd like to see made:

• We'd like to have more explicit errors/warnings when folks use this.set if they had passed a component to await render(...). This will help ensure folks don't fall into a strange situation where they opt-in to use the new model, but copy snippets from the guides or something that tell them to use this.set to mutate in your template.
• We would like to update the RFC to mention adding a "tooling" level API for import { isComponent } from '@glimmer/runtime'; (I think that's the best import path, but maybe @glimmer/manager?). This would significantly reduce some private API usage that the implementation will require.
• We'd like to update the "How We Teach This" to indicate that we will teach this for TypeScript users only (not the default "testing guides"), while we work through a solution that is more ergonomic using named arguments.
• We would like to work through a more ergonomic solution for actually passing arguments into the invoking component.

Passing Arguments Details

test('it works', async function (assert) {
  const args = await renderComponent(
    <template>
      <ProfileCard @name={{@name}} @age={{@age}} />
    </template>,
    {
      name: "Zoey",
      age: 5,
    }
  );

  assert.dom(this.element).hasText('Zoey');

  args.name = 'Zoeyyyyyyyyyyyyy'

  await rerender();

  assert.dom(this.element).hasText('Zoeyyyyyyyyyyyyy');
});

The current design as written in the RFC still fundamentally must work so there is no risk about this RFC introducing something we actively want to remove, but a design like ^ would be a bit nicer to teach in our opinion.

@cafreeman - Iterating this RFC for ^ or just moving forward as written and updating the prose to refer to a future RFC that will add support for leveraging named arguments is totally fine, totally up to you -- we are happy to do whichever you prefer.

[gnclmorais]

I like this proposal, just want to highlight a possible problem and possible way to address it.

I can already see a lot of people forgetting to use this new await rerender(), asserting things before they have been updated, and fight with tests until they (or a pairing partner) notices that await rerender() is missing.

Could we potentially warn users about pending render operations whenever they do an assert.dom… operation, hinting them that they might be missing a await rerender()?

[chriskrycho]

We talked about this at today’s Framework Core team meeting and are moving it forward into Final Comment Period!

[chriskrycho]

We discussed this again at today's Framework meeting and are merging it!