Dynamic Component Hole Discussion

[ef4]

I'm opening this issue to track discussions with the glimmer-vm and ember core framework teams whose help we need to collaborate on this problem.

Problem Statement

1. To make embroider robust under staticComponents: true, we want addon authors to begin porting away from dynamic component invocation patterns that can't be analyzed.

2. We should make sure we're asking them to port to an API that is ergonomic and long-term supported. We shouldn't make them port a second time any time soon.

Motivating Example

Today a lot of popular addons do this:

// hypothetical-button.js
export default class extends Component {
  get labelComponent() {
    return this.args.labelComponent || "hypothetical-button-default-label";
  }
}

{{! hypothetical-button.hbs }}
<button>
  {{component this.labelComponent}}
</button>

Callers of the component can do all of these:

{{! default case: get the default implementation from the addon }}
<HypotheticalButton />

{{! string case: pass the string name of our own component }}
<HypotheticalButton @labelComponent="custom-label" />

{{! definition case: pass the component definition of our own component }}
<HypotheticalButton @labelComponent={{component "custom-label"}} />

How should this addon author refactor so that their code doesn't introduce any static analysis hole? Today we have a hole because we can't be sure what {{component this.labelComponent}} refers to.

Embroider's current state-of-the-art

Today, Embroider has a powerful system of rules for annotating what other code is doing and assisting the user in incrementally tracing where dynamic components come from. We can annotate that the labelComponent argument must be a valid, statically-known component definition, which will cause Embroider to trigger new assertions at the callsites of <HypotheticalButton/>, which can then be resolved through more rules or refactoring, etc, until all dynamic component invocations bottom out on something we can understand statically (example: {{component "a-string-literal"}} is safe and common), or the user opts out of safety and says they'll manage it themselves.

This is only scaffolding. It's not a good experience, and it's fragile because the annotations live outside the packages they're talking about. Rather than create first-party annotations inside addons, I would rather give addons better patterns to refactor into.

The Difficulty Today

For an addon author who wants to do the right thing, there are two awkward problems:

1. We need to make a clear static dependency on the default implementation. The example getter code:

   get labelComponent() {
     return this.args.labelComponent || "hypothetical-button-default-label";
   }

   has an implicit dependency on a component that we can't discover statically. It would be better if we could say:

   import DefaultLabel from './hypothetical-button-default-label';
   export default Class extends Component {
     get labelComponent() {
       return this.args.labelComponent || DefaultLabel;
     }
   }

   but this doesn't work because component classes are not component definitions. Today you can only get a component definition from inside a template, so you'd need to move to move the logic into the template. The best I can come up with is:

   {{#let (or this.args.labelComponent (component "hypothetical-button-default-label")) as |Label| }}
     <Label />
   {{/let}}

   But this adds extra block nesting and noise to templates that can already be quite complex, since we're talking about extensible addon components here. As a refactor, it would be less onerous if they could continue to work in the JS where they were using JS before.

2. There's no good way to only invoke a component definition you've been given without also accepting strings. You might think that refactoring from:

   {{component this.labelComponent}}

   to:

   <this.labelComponent />

   is the right solution, and it probably is over the long run, but right now IT ACCEPTS STRINGS!. And I fear that because this bug has been out long enough, people may even be relying on it, so I don't know how hard it will be to fix. ☹️

In summary, the bad behavior (passing a string) always works and the good behavior (statically importing a component and then invoking it) never works.

Possible Solutions

1. Ideally, we would make component classes invocable. This seems to be a requirement for a coherent world with template imports anyway (importing a component into the template must give you the same value you'd get if you imported it into Javascript first, and the value you import into a template must be invocable).

   However, if we think invocable component classes are far away, we could also ship a getComponentDefinition macro. I would propose it works like:

   import SomeComponent from './some';
   export default class extends Component {
     label = getComponentDefinition(SomeComponent)
   }

   <this.label />

2. Fix the string angle-bracket invocation bug.

   However, if we think that fix is far away or too breaking, an alternative is to ship a dedicated invocation helper that can only invoke component definitions and never strings.

Rollout Concerns

Whatever solutions we create, we need addon authors to be able to begin adopting them immediately without breaking their Ember support matrix. So any new Ember features will need polyfills.

Also, we are asking addons to reduce their public API surface so that they no longer accept string arguments as components. If we want this to be adopted quickly, it would be best to provide tooling that lets the same code work as a deprecation warning in the traditional build and simultaneously an error under Embroider. This may be a good argument for a dedicated invocation helper. It would not be very hard to write on top of the existing embroider macros infrastructure. This:

{{embroider-safely-invoke something}}

Would compile to:

{{#if (macroCondition (macroGetConfig("@embroider/core" "active"))) }}
  {{component (assert-not-string something) }}
{{else}}
  {{component (warn-if-string something) }}
{{/if}}

Next Actions

My main goal here is to get feed back on: what should I tell addon authors to do?

We're going to expand Embroider's Addon Author Guide as soon as we can and I want it to have clear instructions on how to solve this particular case. I can readily invent some embroider-specific libraries for addon authors to incorporate that would work around these problems, but I would much prefer to move them toward the coherent, first-class features the Ember project wants them to be using long term.

[ef4]

An additional option that was not addressed above: we can take the existing packageRules system and allow addons to ship their own first class annotation rules about their own components. I had initially assumed we wouldn't do that because I'd rather have addons port directly to less-dynamic patterns. But letting them ship annotations might make it easier for them to adopt quickly, especially since forbidding their callers from passing strings might be a breaking change for them.

[ef4]

Thankfully the string angle bracket invocation bug is now fixed in ember.

[ef4]

Linking to the related ember issue: emberjs/ember.js#19099

[ef4]

Both these problems are fully addressed upstream in ember now.

Components are first-class invocable values and the angle bracket string invocation bug is long fixed.