search

ionic-team / stencil

A toolchain for building scalable, enterprise-ready component systems on top of TypeScript and Web Component standards. Stencil components can be distributed natively to React, Angular, Vue, and traditional web developers from a single, framework-agnostic codebase.
https://stenciljs.com
Other
12.43k stars 781 forks source link

Rendering not working when view provided by collection of view functions. #4640

Open Bombo opened 11 months ago

Bombo commented 11 months ago

Prerequisites

Stencil Version

4.0.2

Current Behavior

This might be more of an oddity, and our approach may be futile and plain wrong, so this is maybe more of a question than a bug report.

We were looking into ways of having some separation of concerns regarding view and view logic. Our main approach was to have the view be provided via an external function, which takes the "View Model" as its parameter.

/// my-component.ts
import { Component, Prop } from '@stencil/core';
import { format } from "../../utils/utils";
import view from './my-component.view';

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.scss',
  shadow: true,
})
export class MyComponent {
  @Prop() first: string;
  @Prop() middle: string;
  @Prop() last: string;

  getText(): string {
    return format(this.first, this.middle, this.last);
  }

  render() {
    return view(this);
  }
}

///my-component.view.tsx
import { Host, h } from '@stencil/core';
import { MyComponent } from './my-component';

export default function ($: MyComponent) {
  return (
    <Host>
      <div class="test">Hello, World! I'm {$.getText()}</div>
    </Host>
  );
}

This works fine and as expected. One colleague wanted to have a bit more flexibility by providing a collection of view functions instead of just a single function:

/// my-component.ts
[…]
  @Prop()
  view: "first" | "second" = "first";

  render() {
    return ViewCollection[this.view](this);
  }
[…]

/// ViewCollection.tsx
import { Host, h } from '@stencil/core';
import { MyComponent } from './my-component';

export class ViewCollection {
  private constructor() {}
  static first($: MyComponent) {
    return (
      <Host>
        <div class="test">Hello, World! I'm {$.getText()}</div>
        <span>First</span>
      </Host>
    );
  }
  static second($: MyComponent) {
    return (
      <Host>
        <div class="test">Hello, World! I'm {$.getText()}</div>
        <span>Second</span>
      </Host>
    );
  }
}

This does not work, at least not on its own. The resulting html looks like this: image

All of this has been done in the Stencil starter project.

A way to "fix" the issue is to have a second stencil component inside of the project, which uses regular rendering, or the simple function call as shown at the top of this report.

Also something that would fix this would be to add a simple rendering function to the end of ViewCollection.tsx:

import { Host, h } from '@stencil/core';
import { MyComponent } from './my-component';

export class ViewCollection {
  private constructor() {}
  static first($: MyComponent) {
    return (
      <Host>
        <div class="test">Hello, World! I'm {$.getText()}</div>
        <span>First</span>
      </Host>
    );
  }
  static second($: MyComponent) {
    return (
      <Host>
        <div class="test">Hello, World! I'm {$.getText()}</div>
        <span>Second</span>
      </Host>
    );
  }
}

/// this one is new
export function renderView($: MyComponent) {
  return (
    <Host>
      <div class="test">Hello, World! I'm {$.getText()}</div>
      <span>Single</span>
    </Host>
  );
}

That renderView function isn't called anywhere, but its mere existence causes the html to be rendered as expected.

Expected Behavior

Using a view collection as shown above to just work. But of course an explanation as to why that is not possible would also be fine with me. :)

System Info

System: node 16.19.0
    Platform: darwin (22.5.0)
   CPU Model: Apple M1 Pro (10 cpus)
    Compiler: /Users/borisrosenow/test/stencil/stencil-test/node_modules/@stencil/core/compiler/stencil.js
       Build: 1690208707
     Stencil: 4.0.2 😈
  TypeScript: 5.0.4
      Rollup: 2.42.3
      Parse5: 7.1.2
      Sizzle: 2.42.3
      Terser: 5.19.1

Steps to Reproduce

Follow the Get Started guide from the Stencil documentation and change my-component as shown above. Make sure that it's the only component inside of the project.

Code Reproduction URL

https://github.com/Bombo/stencil-test

Additional Information

No response

alicewriteswrongs commented 11 months ago

Hey @Bombo, thanks for filing this issue!

It might be a bit unexpected that Stencil works this way, but this behavior is a result of some essential aspects of how Stencil builds components.

When you run stencil build, under the hood Stencil uses Rollup to bundle together your component code and the Stencil runtime, which makes the functionality available via decorators work and also provides a virtual-DOM based rendering engine.

In order to keep the output size as small as possible, during the build process Stencil does static analysis of the components in your project and keeps track of which features of the Stencil runtime are actually used. The results of this analysis are then injected into the build and Rollup's dead-code-elimination functionality is leveraged to delete code from the Stencil runtime which wouldn't be called by any of the components in the project.

Essentially, what is happening in the example you shared is that the ViewCollection class that you have doesn't look like a Stencil component to Stencil, so the static analysis that it does is wrong and a bunch of code is removed from the built output erroneously because, for instance, Stencil thinks (incorrectly!) that much of the VDom rendering pipeline is not needed.

This isn't happening when you have the little renderView function because that function looks like a Stencil component and therefore it causes the static analysis to return a correct summary of which runtime features are going to be necessary, so the right code is included in the built output.

Another workaround which is pretty similar to what you have but which doesn't need a useless extra component sitting around would be something like this:

diff --git a/src/components/my-component/VIewCollection.tsx b/src/components/my-component/VIewCollection.tsx
index bb3a890..9443f26 100644
--- a/src/components/my-component/VIewCollection.tsx
+++ b/src/components/my-component/VIewCollection.tsx
@@ -1,31 +1,30 @@
 import { Host, h } from "@stencil/core";
 import { MyComponent } from "./my-component";

-export class ViewCollection {
-  private constructor() {}
-  static first($: MyComponent) {
-    return (
-      <Host>
-        <div class="test">Hello, World! I'm {$.getText()}</div>
-        <span>First</span>
-      </Host>
-    );
-  }
-  static second($: MyComponent) {
-    return (
-      <Host>
-        <div class="test">Hello, World! I'm {$.getText()}</div>
-        <span>Second</span>
-      </Host>
-    );
-  }
+export function first($: MyComponent) {
+  return (
+    <Host>
+    <div class="test">Hello, World! I'm {$.getText()}</div>
+    <span>First</span>
+    </Host>
+  );
 }
-
-export function renderView($: MyComponent) {
+export function second($: MyComponent) {
   return (
     <Host>
-      <div class="test">Hello, World! I'm {$.getText()}</div>
-      <span>Single</span>
+    <div class="test">Hello, World! I'm {$.getText()}</div>
+    <span>Second</span>
     </Host>
   );
 }
+
+export const ViewCollection = { first, second }
+
+// export function renderView($: MyComponent) {
+//   return (
+//     <Host>
+//       <div class="test">Hello, World! I'm {$.getText()}</div>
+//       <span>Single</span>
+//     </Host>
+//   );
+// }

basically refactoring those methods into two separate module-level functions with an object to map the view to them (so you could share that between components if you wanted). These functions look like Stencil components to Stencil so the right things end up being included in the build output.

It's possible that there are some issues here where Stencil's static analysis should be working better in the code that you wrote, so I'm going to label this for ingestion into our internal backlog and further analysis.

Thanks again for filing, and I hope that this all makes a bit more sense now!

Bombo commented 11 months ago

Thanks @alicewriteswrongs, this makes total sense to me, and it'll definitely help us structuring our code in such a way that helps us achieve our goal while keeping in mind how Stencil looks at things, so it'll actually work!