fix: text nodes should be taken into account by _render

felippe-regazio commented 1 year ago

_render is not taken text nodes into account. this leads to situations like this: imagine the following component

    return (
      <>
        <h1>{this.data.count}</h1>

        <button onclick={() => this.data.count--}>
          Dec
        </button>

        <button onclick={() => this.data.count++}>
          Inc
        </button>

        { this.props.children }      
      </>
    )

note that the { children } are the last node collection. now imagine that we called this component like this:

  <MyComponent>
      asdf
  </MyComponent>

we will get this result:

<my-custom-element>
    asdf
  <h1>0</h1>
  <button>Dec</button>
  <button>Inc</button>
</my-custom-element>

im having this problem using the "web component mode" because of this part of the code:

        /** customElementsMode.ts - Line 43 **/
        const children = Array.from(this.children).map(c => render(c))

        // because nano-jsx update need parentElement, so DocumentFragment is not usable...
        const el = h(
          'div',
          null,
          _render({
            component,
            props: {
              children: children,
              ref: (r: any) => (ref = r)
            }
          })
        )

the _render will be called and wont see the text node which will be ignored by the handles. obs: im not sure if its possible to use Node.TEXT_NODE here taking different envs into account, but would be the safer way.

yandeu commented 1 year ago

I tried to write a browser test for Web Components (https://github.com/felippe-regazio/nano/commit/39f25bfdbd5223949aadb852cda312ba92aa2463), but since I don't exactly know how it supposed to work, I don't know how to write the test. Maybe you can test the case you mentioned above?

The customElementMode was written by @Shinyaigeek, maybe he can help with some details? And also with https://github.com/nanojsx/nano/pull/130?

felippe-regazio commented 1 year ago

Some help from @Shinyaigeek would be great, for sure. I will wait for his opinion (in case of he asks to change something or have a better ideia about the behavior), an then I will proceed with the tests.

Shinyaigeek commented 1 year ago

I looked at the 39f25bf commit. I feel it is better to register a my-custom-element component by running defineAsCustomElements in the <script /> in the <head />, and write out the my-custom-element component in the body tag as HTML, and then write the test code in the <script /> at the bottom of the <body /> in order to write tests about web-component behavior considering the usual way to use web-components

like:

<html>
  <head>
  ...
  <script src="nanojsx" />
  <script type="module">
  // defineAsCustomElements
  </script>
  </head>
  <body>
  <my-custom-element />
  <script type="module">
  // test code
  </script>
  </body>
</html>

yandeu commented 1 year ago

to write tests about web-component behavior considering the usual way to use web-components

Good idea. Thanks!

nanojsx / nano

fix: text nodes should be taken into account by _render #129