meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
574 stars 159 forks source link

intermittent hooks errors #297

Closed hexsprite closed 4 years ago

hexsprite commented 4 years ago

I've been getting intermittent hooks errors when using useTracker.

Not sure if the issue is somewhere in my own code (usually is haha) or if I uncovered an issue in this package.

The traceback is a little inconsistent and confusing as well. In one traceback it refers to ForwarfRef, in the 2nd it refers to UserMenu (one of the project's components which uses useTracker).

It doesn't happen all the time so it appears that it may be some kind of race condition...

image

hexsprite commented 4 years ago

Also note the Error from tracker recompute: at the bottom. It seems to indicate that the hook is getting CALLED from a Tracker recompute which seems sketchy... i have no idea why that would be

hexsprite commented 4 years ago

ps the referenced UserMenu is very dumb, doesn't use any other hooks other than UserMenu and otherwise renders simple HTML.

CaptainN commented 4 years ago

Can you show how you are using the hook?

Also, what version of React is that?

I see that you are not using deps, so that's maybe something to note.

hexsprite commented 4 years ago
  const { showSubscribe, showProUpgrade } = useTracker(() => {
    let showSubscribe = false
    let showProUpgrade = false
    const user = Meteor.user({ fields: { billing: 1 } })
    if (!Meteor.isCordova && user?.billing) {
      const { billing } = user
      showSubscribe = App.isTrialing() || App.isUnpaid()
      showProUpgrade = !showSubscribe && billing.plan !== 'pro'
    }
    return { showSubscribe, showProUpgrade }
  })

"react": "^16.13.1",

Re: deps - The only reason to change is if some user attribute changed so I didn't think it would need something in the dep part of the function.

CaptainN commented 4 years ago

I agree that you don't need to specify any deps values, but the presence of even an empty deps array does change the behavior of the hook. It might even solve your problem.

However, I'd like to make sure there's not a bug in the hook. There should be no change in the order of internal hook calls within the useTracker hook, unless something screwy happens, like the deps arg swaps between an array and undefined or something like that - but that's not happening here based on your example code.

Try setting deps to an empty array and see if that clears the problem. If it does, it might mean there's a bug in the useTracker implementation. If it doesn't, it still might be a bug, but it could also be in your code. (I don't think we are using ForwardRef for example)

CaptainN commented 4 years ago

What's going on inside App.isTrialing() || App.isUnpaid() - those aren't calling react hooks right?

If either of those are throwing an error, or using hooks, it could explain this.

hexsprite commented 4 years ago

No more usages of hooks.

They just retrieve Meteor.user() and look at some data on it.

So if they throw an error it will get swallowed?

CaptainN commented 4 years ago

It shouldn't swallow the error, but I'm honestly not sure. We should get a unit test or two for that.

I don't see anything in here to explain the error you posted. What version of react-meteor-data are you running? (You can check in .meteor/versions

hexsprite commented 4 years ago

Maybe I'll try throwing an error in there just to see what happens.

react-meteor-data@2.1.2

I noticed a couple other usages of useTracker and some of them should have had some deps actually. And I also changed the ones without deps to use an empty array. I'll followup if this comes back.

hexsprite commented 4 years ago

Still having this since I switched to use deps on all my usages of useTracker. I did notice on one of the tracebacks that it still says NoDeps...

I did a search of some logs and I see that withTracker uses useTracker with the nodeps variant. So now I'm looking over my usages of that.

CaptainN commented 4 years ago

Yes, withTracker does use useTracker - that's why it's important for the nodeps path to work correctly.

If you could, please also check for cases where you might be calling a hook inside the computation itself.

hexsprite commented 4 years ago

continuing to have issues.

Note the latest traceback shows an internal React error that I'm curious about.

Having poured over my application code I'm relatively certain that it has nothing to do with breaking the rules of hooks.

It seems to get triggered when a reactive re-rendering occurs.

image

as I mention in the video, the useAsync is a red herring. It just breaks somewhere else if I comment out the useAsync. If I comment out the getTracker calls all is well.

I did this little video walk through that may shed some light. Any ideas on direction to go for debugging this?

https://www.loom.com/share/0901e081c07a4876bc0e309caf7d144e

hexsprite commented 4 years ago

Tried to create a repro using the blaze todos example app and adding a couple react components mounted using the react blaze helper. Wasn't able to trigger the bug. Now I'm gonna try turning off components one by one in the app until maybe I find something.

hexsprite commented 4 years ago

Looks like I may have found the issue. I was doing something like:

let EditComponent
if (something) {
  EditComponent = require('componentA').default
} else {
  EditComponent = require('componentA').default
}
return <EditComponent />

But apparently React did not like that!

CaptainN commented 4 years ago

You should definitely do your requires in the module scope, or at least, never in your render function.

Still, you should be able to conditionally use one component or the other. This may be an error in React, as the error message you mentioned in the video suggests.

hexsprite commented 4 years ago

That wasn't what I was doing before...

I had a Blaze Template like:

{{React component=component subcomponent=subcomponent}}

Then the react component was essentially:

function MyComponent (props) {
  const SubComponent = props.subcomponent;
  return <SubComponent />
}

So the subcomponent was passed as a prop to the main component

Not sure why that caused a (intermittent) React bug...

CaptainN commented 4 years ago

My guess is it was sending a different component (and render method) in some edge cases (maybe the component is being reuses for different routes, or something, and only changing the subcomponent).

It's still somewhat mysterious, but if it was solved this way, it's probably not a bug with useTracker. If you agree, please close this ticket.

I'm glad you found resolution. :-)

hexsprite commented 4 years ago

Thanks! Yeah I hope that's the source. If anything comes up I'll reopen :)

CaptainN commented 4 years ago

In thinking about this, it's possible it is a bug in Blaze-React, but it may not be work-outable in the framework. If you set a key on the subcomponent instance in React, it may solve the problem as well by causing a full react lifecycle when the subcomponent changes. But I'm not sure how you'd approach that.

function MyComponent (props) {
  const SubComponent = props.subcomponent;
  return <SubComponent key={props.someValueWhichIndicatesSubcomponentChanged} />
}
hexsprite commented 4 years ago

Interesting. Though in this case the subcomponent would not change within the lifetime of the component. When instantiated it would stay that kind.

The actual use case was for an Editable component which shows a simple label but when clicked shows an edit widget of some kind (textarea or autocomplete field specifically)