atomicojs / atomico

Atomico a micro-library for creating webcomponents using only functions, hooks and virtual-dom.
https://atomicojs.dev
MIT License
1.16k stars 43 forks source link

useEffect(fn) does not run multiple times #24

Closed dy closed 4 years ago

dy commented 4 years ago

Faced that issue in debugging unihooks. Could not create a sandbox due to #23, so just show the code here.

import { h, customElement, useEffect, useState } from "atomico";

let log = []

function X() {
  // this effect is run only once
  useEffect(() => log.push(1))

  let [s, setS] = useState(0)
  useEffect(() => setS(1), [])
  useEffect(() => setS(2), [])

  return null
}

customElement('x-x', X);
document.body.appendChild(document.createElement("x-x"));

This code runs first useEffect only once.

dy commented 4 years ago

Investigating, likely not atomico issue...

UpperCod commented 4 years ago

Hi, Atomico works under the assumption that if there is a return this must be a function

dy commented 4 years ago

It doesn't work regardless if there's a return statement:

let { h, customElement, useEffect, useState } = await import("atomico");

  let log = []

  function X() {
    let [s, setS] = useState(0)

    // this effect is run only once
    useEffect(() => console.log(1))

    useEffect(() => { setTimeout(() => setS(1), 10) }, [])
    useEffect(() => { setTimeout(() => setS(2), 20) }, [])

    return null
  }

  customElement('x-x', X);
  document.body.appendChild(document.createElement("x-x"));

The console shows 1 single time only. Same is with useMemo

UpperCod commented 4 years ago

Hi, thanks for your issue, it gives me space to respond to interesting Atomico behavior.

for now the use of null eliminates the web-components of the document, since the web-components was removed, Atomico avoids updates that remain pending since in case the web-components no longer exists.

Any activity that is pending once the web-components have been unmounted, is ignored with the intention of not overloading the browser due to phantom effects.

dy commented 4 years ago

Yes, returning h('') from component makes it work with useEffect, but not with useMemo:

let { h, customElement, useState, useEffect, useMemo, createHookCollection } = await import("atomico");

  function X() {
    let [s, setS] = useState(0)

    // this memo is run only once
    useMemo(() => console.log(s))
    useMemo(() => setTimeout(() => setS(2), 100), [])
    useMemo(() => setTimeout(() => setS(3), 200), [])

    return h('')
  }

  customElement('x-x', X);
  document.body.appendChild(document.createElement("x-x"));
UpperCod commented 4 years ago

Hi, sorry for the delay in my response, I was traveling ... the problem is that the Atomico virtual-dom does not consider h('') as valid, it is advisable to always return a node or the host tag

dy commented 4 years ago

But that doesn't work with h('abc'), h('host'), h('div') or any other. useMemo is simply broken - it does not trigger with empty deps.

UpperCod commented 4 years ago

Now I understand the error you are telling me, useMemo does not follow the React style, in the following case useMemo(() => console.log(s))...

This is Atomico's own behavior, useMemo is for executions that can be blocked by a list of arguments, by default Atomico fills the second parameter of useMemo with an empty array, avoiding the need to declare this as such.

https://github.com/atomicojs/atomico/blob/master/src/core/hooks.js#L141

Do you think it is better to avoid this to maintain an exact resemblance to React? ...

dy commented 4 years ago

Do you think it is better to avoid this to maintain an exact resemblance to React?

I believe that is critical. React hooks, although may look opinionated, have very fine-tuned design. useMemo in particular is the only way to run synchronously deps-gated code. If you treat it on your own manner, not only that is misleading, but also very likely breaks dependent hooks libraries, which is the case of unihooks - that way many userland hooks ported from react turn out to be broken. I believe the "core" hooks (useState, useEffect, useReducer, useRef, useMemo, useCallback, useContext, useLayoutEffect) must behave the same way as react hooks (or as close as possible).

UpperCod commented 4 years ago

the new version atomico@0.18.0 corrects this, removed the expression function useMemo(callback, args = ARRAY_EMPTY), this match the behavior with react, I hope you find it useful