node.setAttribute is not a function

timonson commented 4 years ago

When I use your link import { h, patch } from "https://unpkg.com/superfine" I get the following error:

Uncaught TypeError: node.setAttribute is not a function
    at patchProperty (index.js:38)
    at createNode (index.js:52)
    at createNode (index.js:56)
    at patchNode (index.js:146)
    at patch (index.js:264)
    at (index):103

It does not happen when I import the file from my device. Do you need more information?

jorgebucaran commented 4 years ago

@timonson Would you mind sharing what the problem was and whether you fixed it or not? :)

timonson commented 4 years ago

@jorgebucaran

I was not really able to figure out what might cause the error. Strangely enough, I wrote my own h function and replaced only this function in your library and it worked. But I don't know why it reacts differently.

I am writing my own css-in-js library with the goal to to use it together with hyperapp and superfine but I can't find the reason why it works with my h function but not with yours'. You can find the library here and I would appreciate it highly if you would check it out. All you have to do is going to the examples folder and run npx live-server. In the same folder you find a file called superfine.js which has my own h function which I mentioned earlier, but the rest is the same. Maybe you have an idea what the causation of the error could be?

Thank you!

Edit1: Let me know if you want me to close this issue because it is not clear that this is an issue with superfine at all.

Edit2: Here is my h function where this error does not occur:

function h(name, props, ...rest) {
  const children = rest
    .filter(vnode => vnode !== false && vnode !== true && vnode != null)
    .flat(Infinity)
    .map(vnode => (typeof vnode === "object" ? vnode : createTextVNode(vnode)))

  props = props || {}
  return typeof name === "function"
    ? name(props, children)
    : createVNode(name, props, children, null, props.key)
}

jorgebucaran commented 4 years ago

@timonson Can you create a CodePen or provide an example where Superfine fails?

timonson commented 4 years ago

@jorgebucaran After way too many hours I found the root cause for the error. I always assumed it was a scope issue but could not identify it exactly. It is literally only one word in line 290: EMPTY_OBJ. The reason why it causes trouble in my case is that EMPTY_OBJ is defined globally. When I change it to {} inside the function, so that props has local scope, it works. I will give you tomorrow an exact applied case where this causes trouble so that you will maybe consider changing it. The same applies to Hyperapp by the way.

jorgebucaran commented 4 years ago

@timonson Thanks! This could be a bug.

I will give you tomorrow an exact applied case where this causes trouble so that you will maybe consider changing it.

Can you make a demo in CodePen or CodeSandbox where I can see the error message?

timonson commented 4 years ago

@jorgebucaran I was not able to make codepen run my code but I made an html file which you can find here. This code imports two ES Modules - one from your hyperapp and the other one from this file. It definitely runs in google-chrome browsers until the error node.setAttribute is not a function appears.

The reason for the error is that EMPTY_OBJ in hyperapp is defined globally and later - in the h function - the object is assigned to the props variable. In my case, my library stickit, this reference to the object is changed and this is how the error is caused. You could change it by defining the objects inside the functions or just by assigning a {} directly inside the functions instead of the global variables. I will make a pull request and you can decide afterwards.

jorgebucaran commented 4 years ago

@timonson If you can't make a minimal working example reproducing the bug, a PR would be the second-best thing you can do! :)

In my case, my library stickit, this reference to the object is changed and this is how the error is caused.

You probably shouldn't do that. Think of it as a "warranty void if removed".

timonson commented 4 years ago

@jorgebucaran Here is a minimal working example causing the error:

<!DOCTYPE html>
<html lang="en">
  <head>
    <script type="module">
      import { h, patch } from "https://unpkg.com/superfine"
      const node = document.getElementById("app")
      const setState = state => {
        patch(
          node,
          h("div", null, [
            h(Button, null, "Click"),
            h(Button, { className: "my-button" }, "Click"),
            h(Button, { className: "my-button" }, "Click"),
            h(UlCircles, null, [
              h("li", null, "uno"),
              h("li", null, "dos"),
              h("li", null, "tres"),
            ]),
          ])
        )
      }

      function foo(element) {
        return function(props, children) {
          props.className = props.className
            ? `${props.className} anotherClass`
            : "anotherClass"
          return h(element, props, children)
        }
      }

      const Button = foo("button")
      const UlCircles = foo("ul")
      const UlCirclesAndColored = foo("ul")
      const UlCirclesAndColoredGreen = foo("ul")

      setState(0) // Start app with initial state.
    </script>
  </head>
  <body>
    <div id="app"></div>
  </body>
</html>

Now, when you change the second line of the foo function to

      function foo(element) {
        return function({ ...props }, children) {
          props.className = props.className
            ? `${props.className} anotherClass`
            : "anotherClass"
          console.log(props)
          return h(element, props, children)
        }
      }

... then the error does not appear, because we have lost the reference to EMPTY_OBJ by creating a totally new Object via { ...props }. It is not a big deal, but it is just one of these annoying things global scope can cause sometimes. I made a pull request.

jorgebucaran commented 4 years ago

@timonson The solution is simple: don't mutate the props object. Also, I recommend using class, not className, but that's another thing. Here's how you can rewrite your foo function to not mutate props.

const foo = element => (props, children) =>
  h(
    element,
    { ...props, class: `${props.class || ""} anotherClass` },
    children
  )

It is not a big deal, but it is just one of these annoying things global scope can cause sometimes.

Global scope can cause all sorts of problems, but this is only an implementation detail. You should never mutate objects you don't own.

jorgebucaran / superfine

node.setAttribute is not a function #176