WebReflection / uland

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

Can't use hooks in memorized components #12

Closed jaschaio closed 3 years ago

jaschaio commented 3 years ago

Hey there, I was trying to do some performance tests with createContext, useContext and useMemo and found a weird error.

index.js:302 Uncaught TypeError: Cannot read property '$' of undefined
    at useMemo (index.js:302)
    at pen.js?key=pen.js-0f4efd8f-a160-62c6-5923-a2bdc5949a3f:65
    at index.js:1379
    at hook (index.js:237)
    at index.js:451
    at unroll (index.js:1413)
    at unrollValues (index.js:1437)
    at unrollHole (index.js:1422)
    at index.js:1383
    at hook (index.js:237)

This happens as well when I try to use a hook in a memorized component in neverland. Here is a fork of your useContext codepen using neverland: https://codepen.io/jaschaio/pen/vYgZKKx?editors=0010

Check line 54 where I simply added a dummy useState hook to the Fruit Component:

const Fruit =  component(memo((({label}) => {

  const [ test, setTest ] = useState(); // <!-- dummy hook which throws an error

  return html`
  <div class="fruit">last render: ${Math.round(performance.now())} ${label}</div>
` } )));

This is the error neverland throws:

Uncaught TypeError: Cannot set property 'args' of undefined
    at useReducer (index.js:362)
    at useState (index.js:389)
    at pen.js?key=pen.js-f5e7655f-655c-bbd1-a57b-70c15a903700:54
    at pen.js?key=pen.js-f5e7655f-655c-bbd1-a57b-70c15a903700:16
    at useMemo (index.js:513)
    at pen.js?key=pen.js-f5e7655f-655c-bbd1-a57b-70c15a903700:16
    at index.js:1719
    at Object.hook (index.js:309)
    at Object.hook (index.js:578)
    at unroll (index.js:1767)

If I exchange neverland for uland in the same codepen I get this error instead:

index.js:387 Uncaught (in promise) TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)
    at set (VM10310 index.js:387)
    at wrap (VM10310 index.js:392)
    at useState (VM10310 index.js:423)
    at VM10312 pen.js:54
    at VM10312 pen.js:16
    at useMemo (VM10310 index.js:299)
    at VM10312 pen.js:16
    at VM10310 index.js:1379
    at hook (VM10310 index.js:237)

The original error I encountered above was within this codepen: https://codepen.io/jaschaio/pen/JjEJXvQ?editors=0110

If I remove the useRenderCounter (e.G. use no hooks) from the Counter and DisplayCount Components no error is thrown. If I add an arbitrary useState hook the same error is thrown.

What I was originally intending to do was replicating the example given here and see what kind of effect useContext and useMemo have on rendering my component tree.

WebReflection commented 3 years ago

You can't create a component out of a hook, a hook can exist only inside a component. Also note that context provide triggers a hook update each time, so having it within the hook creates an infinite loop.

Context is a bit different in uland but as there's no pre-processor here, not sure what you'd expect there invoking context provide right away on top of your app ... regardless, useMemo to create a component is a no-go, place it inside a callback or see failures happening.

Same goes with uland exmple ... you can't invoke withStore and use hooks outside components ... think it like this: unless the utility is used inside a component, you cannot use hooks because there is no top level hook in here, it's your App the entry point.

If you could show a smaller example and tell me what is it that you are trying to do, I might try to help more, but right now all behaviors are expected: hooks work only inside components.

WebReflection commented 3 years ago

P.S. if this example is what you are after, I can recreate it in code-pen. Let me know, thanks.

jaschaio commented 3 years ago

Thanks for your input!

But honestly – I am confused.

regardless, useMemo to create a component is a no-go, place it inside a callback or see failures happening.

Isn't this exactly what you are doing in your useContext codepen demo? I actually only got the idea of using useMemo to create a component from that codepen you have linked to in the neverland repository: https://github.com/WebReflection/neverland/blob/master/codepen.md

As there is little information on how to use useContext within neverland/uland and the differences to react I mainly used that simple codepen to guide myself.

Anyway,

If this example is what you are after, I can recreate it in code-pen. Let me know, thanks.

Yes, that's what I was trying to recreate. So maybe having a look at how you would do that would already help a lot.

Thanks again

WebReflection commented 3 years ago

Isn't this exactly what you are doing in your useContext codepen demo?

Maybe I overlooked at your example, but memo in my codepen returns a callback, not a component, so you can't use useMemo directly to create a component, which was my point, but indeed that part looks the same in your code pen.

As I've checked closer the difference, indeed the only issue you have is that useState runs conditionally, and conditional hooks are not allowed even in React, as far as I know.

All hooks must be known during execution, so that the following breaks:

if (condition)
  const [what, ever] = useState();

When you memoize a callback it runs the first time, but not necessarily next times ... and there you have a problem, but it's not a uland issue, it's how hooks work.

Maybe this post could help understanding hooks better, or even this older one, which explains why conditional hooks are forbidden, and that's in React, as well as in uhooks.

useMemo creates that conditional case you don't want ... but yeah, everything else looks indeed fine.

Does this help?

WebReflection commented 3 years ago

early post, sorry, let me move forward ...

Yes, that's what I was trying to recreate. So maybe having a look at how you would do that would already help a lot.

"hold my beer!" (coming soon)

WebReflection commented 3 years ago

@jaschaio gotta be honest, I don't understand what that gist is trying to solve, as all components are related to the same context, hence I expect these to update whenever such context changes ... right?

Anyway, this to me works as expected: https://codepen.io/WebReflection/pen/KKavQym?editors=0010

I am not sure why you want to not have components knowing about the context provider being changed/triggered, but the thing is that in uhtml/uland renders are cheap, and having always the expected latest state around is less error prone, imho, but also ... since renders are cheap, you shouldn't care.

Components don't get update neither, only parts that should, so in this case you never need to useMemo, imho, but maybe I am misunderstanding ... all I'd like to underline one more time: if you want 1:1 React behavior, the only library that will give you that is, indeed, React 👍

jaschaio commented 3 years ago

Thanks, I appreciate it.

I guess the gist is obviously oversimplifying. But let's take a real world app that has dozens of different contexts and hundreds of different components at the same time.

If I just call context.provide for each context at the top of the App then all of my components will rerender whenever even a single value within any of the contexts changes. renders might be cheap, but if we have a lot of different components that for example compute styles based on passed in properties it can still become expensive. Or at least that has been the experience I had with hyperthtml (and redux instead of using context)

Or do you remember why you combined useContext with useMemo in your original useContext demo codepen?

the only issue you have is that useState runs conditionally, and conditional hooks are not allowed

Anyway, I obviously did a mistake in calling the hook conditionally. But this still doesn't explain why I wasn't able to use the useState hook within your forked example useContext codepen:

image

The only difference to your original is the dummy useState call within the fruit component itself.

WebReflection commented 3 years ago

If I just call context.provide for each context at the top of the App then all of my components will rerender whenever even a single value within any of the contexts changes.

Maybe I don't get React meaning of the context, but here when you provide a value, and such value is different from the previous one, all components that used that context get updated, and that's expected.

Here the context changes value, so why wouldn't inner components render that value, once updated? This is the part I don't get about that gist, the goal looks more like a bug in the context logic ... or better, why associating 3 components to the same context, if inner components should not render when such context provides a new value? What's the meaning of context at all then?

Anyway, I obviously did a mistake in calling the hook conditionally. But this still doesn't explain why I wasn't able to use the useState hook within your forked example useContext codepen:

Using useState within a meomized callback is the conditional error I was talking about. Each hook needs to always run, so unless your memo is guarded by a value used as state inrternally, using useState inside a meomized callback with no guards is an anti-pattern, and AFAIK that's the case in React too.

However, it's possible that my memo old demo invokes the callback regardless and return the meomized callback only once ... I need to remember why that demo does that, and what was the idea behind.

Regardless, in uhtml rendering cost nothing unless you indeed pass inline styles all over, but arguably that's what you shouldn't do in the first place, and yet, I'd like to see claim where hyperHTML is slow, or uhtml, or uland, because last time I've checked, they are minimum twice as fast as React in simple to convoluted conditions, and we use HyperHTMLElement with thousands of filters rendered in a table with highly dynamic features that change all time and zero issue on million target browsers and platforms.

As summary

Thanks.

jaschaio commented 3 years ago

Thanks Andrea. Again please forgive naive or even outright stupid questions – as I have said previously you are probably a 10x more skilled developer then the average end user of your libraries.

I am honestly just looking for a solid pattern to use for porting my Redux + hyperhtml.Component based App to uland and useContext. Your original example using useMemo and my recent experience working with react was pretty much all I could base this on as I there is little previous discussion about useContext in both uland and neverland.

I would have 30 or so different stores created with useReducer and a deeply nested Component Tree. Some branches of the tree care about some of the stores, and others don't. Sometimes two branches or components in different branches might care about the same store. I obviously only want to render branches or components that do care about their particular store.

So maybe I should stop worrying too much about renders unless there is something obviously wrong like passing inline styles over and over.

As you say that you would:

provide new context only when/if needed, or when other things change

I guess I could use an useEffect hook that calls context.provide guarded by the store value.

const createStore = () => {

  const [ store, dispatchStore ] = useReducer( reducer, initialState );

  if ( ! StoreContext )
    StoreContext = createContext( [ store, dispatchStore ] );

  useEffect( () => StoreContext.provide( [ store, dispatchStore ] ), [ store ] );

}

I could then just call createStore as far down the component tree as I start caring about the particular store. As StoreContext.provide triggers a hook update anyway, other components in adjacent trees would still rerender as the context updated itself.

Would this be an acceptable pattern?

Here is a codepen for that: https://codepen.io/jaschaio/pen/oNBGbdG?editors=0010

One thing I am wondering about: Why does clicking on the Dispatch nothing button (e.G. dispatching and empty action to the reducer) still triggers a render? I guess this is the same as the const [, forceUpdate] = useState(); call in the latest codepen you shared – just found curious about the inner workings.

WebReflection commented 3 years ago

Nothing else gets rendered, really, except for the textContent of each reference, but that's expected:

  useEffect(() => {
    ref.current.textContent = Number(ref.current.textContent || '0') + 1
  });

If you don't want that to ever change, just guard useEffect with an empty array (or a guard, see demo):

  useEffect(() => {
    ref.current.textContent = Number(ref.current.textContent || '0') + 1
  }, [] /* HERE */);

The useEffect runs each time a render is called, and a render is always called per each hook executing. Pass a reference or something else to guard effects you don't want to trigger, and that should be it?

Maybe check this example out: https://codepen.io/WebReflection/pen/NWdarja?editors=0010

Dispatch nothing does indeed nothing now, I hope that helps.

WebReflection commented 3 years ago

P.S. you are focusing on the reference render here ... but components that don't change a thing in their content don't really get touched ... you can analyze the DOM and see that only references get updated, not components ... unless you change the input!