WebReflection / uland

A µhtml take at neverland
ISC License
108 stars 2 forks source link

useEffect & useState bug #9

Closed jaschaio closed 3 years ago

jaschaio commented 3 years ago

If I use state within useEffect it always uses the initial state:

const { render, html, useState, useEffect, Component } = uland;

const App = Component( ( props ) => {

  const [ name, setName ] = useState( 'Anonymous' );

  const [ seconds, setSeconds ] = useState( 0 );

  useEffect( () => {

    const timer = setTimeout(() => setSeconds( seconds + 1 ), 1000);

    return () => clearTimeout(timer);

  }, [ seconds, setSeconds ] );

  return html`
    <h1>Hello ${ name }</h1>
    <p>You have been ${ seconds } on this page.</p>
    <label>Your name is:
      <input type="text" value=${ name } onInput=${ ( event ) => setName( event.target.value ) } />
    </label>`

} );

render( document.body, App() );

Codepen: https://codepen.io/jaschaio/pen/XWjeyXL

These even happens in an example you wrote if I change neverland for uland: https://codepen.io/WebReflection/pen/YzzRxOy

Related neverland issue: https://github.com/WebReflection/neverland/issues/38

jaschaio commented 3 years ago

Using a callback works so not sure if this is a bug or a feature but I am confused as it works with neverland

const timer = setTimeout(() => setSeconds( prev => prev + 1 ), 1000);
WebReflection commented 3 years ago

Thanks, I forgot to update callback and guards on useEffect so it was obviously failing at updating new values. It works now 👋

jaschaio commented 3 years ago

Thanks for the quick fix!

One more thing I noticed while doing some experiments with async effects (but not related to the async values itself):

If I add the second value returned from useState to the dependency array of useEffect I get weird results (effects being run on every render):

const { render, html, useState, useEffect, Component } = uland;

const waitWith = ( delay, value ) => new Promise( ( resolve, reject ) => setTimeout( () => resolve( value ), delay ) );

const Icon = Component( ( props ) => {

  const [ icon, setIcon ] = useState( null );

  useEffect( () => {

      console.log( "running effect" );

      var getIcon = async () => {

        const value = await waitWith( 3000, 'testing' );

        setIcon( value );

      }

      getIcon();

  }, [ icon, setIcon ] );

  return html`<span>${ icon }</span>`;

} );

const App = Component( ( props ) => {

  const [ name, setName ] = useState( 'Anonymous' );

  const [ seconds, setSeconds ] = useState( 0 );

  useEffect( () => {

    const timer = setTimeout(() => setSeconds( seconds + 1 ), 1000);

    return () => clearTimeout(timer);

  }, [ seconds, setSeconds ] );

  return html`
    <h1>Hello ${ name }</h1>
    <p>You have been ${ seconds } seconds on this page.</p>
    <p>Your Icon is: ${ Icon() }</p>
    <label>Your name is:
      <input type="text" value=${ name } onInput=${ ( event ) => setName( event.target.value ) } />
    </label>`

} );

render( document.body, App() );

Codepen: https://codepen.io/jaschaio/pen/XWjzMmd?editors=0010

It works if I just add the state variable (e.G. seconds or icons to the useEffect array) but as far as I am aware adding the setState function as well is a common pattern used by React/Preact.

If I compare it directly with React/Preact it works as intended: https://codepen.io/jaschaio/pen/oNzoZzo?editors=0010

WebReflection commented 3 years ago

setIcon is always different... and I guess this fails in augmentor/never land too, right?

WebReflection commented 3 years ago

actually no ... the callback to set state is always the same so I need to take a better look at your example, however, it makes no sense to pass setXXX as guard, as you're interested in the state value, not its updater, so using [icon] and [seconds] is what I'd do regardless.

WebReflection commented 3 years ago

OK, the callback now is always the same, as it is for uhooks in general, so your example works as expected now.

I still would use only the state value as guard, but I guess in React/Preact even the callback is not granted to always be the same, hence the extra guard ... well, I've made it working as expected now.

jaschaio commented 3 years ago

Thanks Andrea! Just doing some early tests and sharing my observations in case it helps to uncover something unintended. I agree with you that adding only the state variable makes most sense

WebReflection commented 3 years ago

No, thank you for these detailed bugs, it helped a lot! 👍