Initial implementation of renderComponent #20781

wycats · 2024-10-22T00:59:57Z

There's more in this commit than there should be, largely because this
commit bundles a change to the plugins that fixes lexical scope bugs.

I intend to separate those changes out into their own PR before
attempting to merge this one. This PR also needs a flag for the new API.

Finally this commit adds some new testing infrastructure to generalize
the base render tests so it can be used with templates that are not
registered into a container. This is useful more generally, and could
be used in other places in the test suite in the future.

There's more in this commit than there should be, largely because this commit bundles a change to the plugins that fixes lexical scope bugs. I intend to separate those changes out into their own PR before attempting to merge this one. This PR also needs a flag for the new API. Finally this commit adds some new testing infrastructure to generalize the base render tests so it can be used with templates that are not registered into a container. This is useful more generally, and could be used in other places in the test suite in the future.

NullVoxPopuli · 2024-10-23T15:48:13Z

packages/@ember/-internals/glimmer/lib/renderer.ts

-  private _owner: InternalOwner;
-  private _context: CompileTimeCompilationContext;
-  private _runtime: RuntimeContext;
+export class RendererState {


how different is all this from how Ember renders an app? 🤔

ah I see: https://github.com/emberjs/ember.js/pull/20781/files#diff-cdf95f0c6c10226a54fbac271478aaa1defd908f0f998fec0f484e52d2544184R603 and https://github.com/emberjs/ember.js/pull/20781/files#diff-cdf95f0c6c10226a54fbac271478aaa1defd908f0f998fec0f484e52d2544184L492 <- original code here, and a lot is added

For the record, I think that a lot of this could be factored to share more code. But when I tried to do more than I did here, I kept running into subtle assumptions I was missing in the original code, so I decided to share the obvious stuff and allow more sharing to happen incrementally once people have time to take a careful inventory of the expected behavior of the original code.

NullVoxPopuli · 2024-10-23T15:48:42Z

packages/@ember/-internals/glimmer/lib/renderer.ts

+ * This function returns `undefined` if there was an error rendering the
+ * component.
+ *
+ * @fixme restructure this to return a result containing the error rather than


what's the timeline of this fixme? 😅

I intended it as a pre-merge note to decide whether it should be fixed or accepted as tolerable. It should still be addressed one way or another before merging.

NullVoxPopuli · 2024-10-23T15:50:01Z

packages/@ember/-internals/glimmer/lib/renderer.ts

+ * @fixme restructure this to return a result containing the error rather than
+ * undefined.
+ */
+export function renderComponent(


would we also want a hydrateComponent? (🙈 what fastboot does )

(this would make Astro's runtime behavior with ember super easy)

I think that's sensible. Maybe it should just be an option like it is in Glimmer: do you want the client-side element builder or the rehydrating one. That's the only difference in behavior between hydrating and normal rendering.

NullVoxPopuli · 2024-10-23T16:06:41Z

packages/@ember/-internals/glimmer/lib/renderer.ts

-  private _owner: InternalOwner;
-  private _context: CompileTimeCompilationContext;
-  private _runtime: RuntimeContext;
+export class RendererState {


ah I see: https://github.com/emberjs/ember.js/pull/20781/files#diff-cdf95f0c6c10226a54fbac271478aaa1defd908f0f998fec0f484e52d2544184R603 and https://github.com/emberjs/ember.js/pull/20781/files#diff-cdf95f0c6c10226a54fbac271478aaa1defd908f0f998fec0f484e52d2544184L492 <- original code here, and a lot is added

NullVoxPopuli · 2024-10-23T16:07:08Z

packages/@ember/-internals/glimmer/lib/renderer.ts

    } else {
      throw new Error(
        'Accessing `this.element` is not allowed in non-interactive environments (such as FastBoot).'
      );
    }
  }

-  getBounds(view: View): {
+  getBounds(component: View): {


what's a "view"? 😅 🙈

TL;DR almost every use of the word "view" means "classic component". I considered renaming all view to classicComponent but it was expanding the scope of this PR a lot. That said, it's actually really clarifying when you see an internal API that literally only takes classic components. It means it must not be all that fundamental to the rendering process since empirically we actually do render a lot of Glimmer components.

That said, it also illustrates a way in which route templates are special: they rely on mounting via APIs on the renderer, and (before this PR) there was literally no way to mount a root component or an outlet component in Ember that was implemented as a Glimmer component.

NullVoxPopuli · 2024-10-23T16:08:24Z

packages/@ember/-internals/glimmer/lib/renderer/strict-resolver.ts

+/**
+ * Resolution for non built ins is now handled by the vm as we are using strict mode
+ */
+export class StrictResolver implements VMRuntimeResolver, VMCompileTimeResolver {


why do we need a resolver at all? can we not have one? 😅

like, why do we still need lookupHelper?

Originally, I thought: I want Glimmer to support the "no resolver" case, and then we wouldn't need one. We'd still want it for now to avoid blocking this PR on that Glimmer change.

As I was implementing this feature, though, I realized that keywords like array and hash and builtins used by AST transformations are not implemented as either Glimmer keywords or AST transformations. Instead, many of them are implemented as custom helpers looked up through the resolver. This is definitely wrong, but it's another thing I don't want to try to fix at the same time as the rest of this PR.

NullVoxPopuli · 2024-10-23T16:11:21Z

packages/@ember/-internals/glimmer/tests/integration/components/render-component-test.ts

+    run(() => {
+      this.component = renderComponent(component, {
+        owner,
+        env: { document: document, isInteractive: true, hasDOM: true },


what happens when isInteractive is false?

When isInteractive is false, modifiers don't run. When hasDOM is true, we default to getting document (and I think other browser-only globals) from globalThis. When it's false, you're required to explicitly provide them.

I think isInteractive makes sense (and exists in Glimmer), but hasDOM feels like a hack that escaped the lab and should be replaced with being explicit in the internals and using the default browser behavior at the edge.

NullVoxPopuli · 2024-10-23T16:12:49Z

packages/@ember/-internals/glimmer/tests/integration/components/render-component-test.ts

+
+    '@test Can shadow keywords'() {
+      let ifComponent = defineComponent({}, 'Hello, world!');
+      let Bar = defComponent('{{#if}}{{/if}}', { scope: { if: ifComponent } });


I understand that this is how strict mode is supposed to work, but I hope no one does this in a real project.

Could allow us to make some good memes tho, like how Cish folks redefine true to be false or whatever

It's also impossible to represent using <template>, since <template> populates the scope with names in the JavaScript lexical scope, and if is a keyword. I don't think we ever specified this, but I assume that the content tag syntax is only expected to work in modules, or you'd be able to define a yield variable and shadow the {{yield}} keyword :yikes:.

NullVoxPopuli · 2024-10-23T16:14:58Z

packages/@ember/-internals/glimmer/tests/integration/components/render-component-test.ts

+    }
+
+    '@test Can use concat'() {
+      let Root = defComponent('{{(concat "Hello" ", " "world!")}}', { scope: { concat } });


wrapping () shouldn't be needed

NullVoxPopuli · 2024-10-23T16:15:48Z

packages/@ember/-internals/glimmer/tests/integration/components/render-component-test.ts

+
+    '@test Can use get'() {
+      let Root = defComponent(
+        '{{#let (hash value="Hello, world!") as |hash|}}{{(get hash "value")}}{{/let}}',


same here with get, {{get hash "value"}} should be equiv

unless I am missing what is being tested here 🙃

(or if this just isn't an important distinction)

This test is explicitly testing the get keyword, so I had to come up with an example that uses get. ;)

NullVoxPopuli · 2024-10-23T16:17:41Z

packages/@ember/-internals/glimmer/tests/integration/components/render-component-test.ts

+    }
+
+    // Ember currently uses AST plugins to implement certain features that
+    // glimmer-vm does not natively provide, such as {{#each-in}}, {{outlet}}


I don't know if I've gotten an answer on this -- but shouldn't the VM have each-in? how important is it where built ins live?
if we put more builtins in ember (say: on, hash, fn, array, element), are we saying that Glimmer shouldn't be used without ember?

I think the VM should indeed have each-in. I also think that implementing these things as AST plugins is causing a lot more trouble than we seem to have realized. For now, I decided to document and clean up the behavior, but I agree strongly with the implication of your various comments that we should move a lot of the nuts-and-bolts implementations of low-level things into the VM.

wycats added 2 commits October 21, 2024 17:59

Fix test types

3a6e06b

NullVoxPopuli reviewed Oct 23, 2024

View reviewed changes

NullVoxPopuli approved these changes Oct 23, 2024

View reviewed changes

wycats marked this pull request as ready for review November 8, 2024 18:56

vstefanovic97 mentioned this pull request Dec 12, 2024

[New Framework] Ember withastro/astro#350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of renderComponent #20781

Initial implementation of renderComponent #20781

wycats commented Oct 22, 2024

NullVoxPopuli Oct 23, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

NullVoxPopuli Oct 23, 2024

NullVoxPopuli Oct 23, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

NullVoxPopuli Oct 23, 2024

wycats Oct 25, 2024

Initial implementation of renderComponent #20781

Are you sure you want to change the base?

Initial implementation of renderComponent #20781

Conversation

wycats commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment