meteor / react-packages

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

react-meteor-data: withTracker() fails under React.StrictMode #278

Closed robraux closed 4 years ago

robraux commented 4 years ago

react-meteor-data version: 2.0.0

Isolated reproduction: https://github.com/robraux/react-meteor-data-failure/ PR introducing the issue: https://github.com/meteor/react-packages/pull/273

When the withTracker component for version 2.x is wrapped in , behavior of the component changes. Props are not made available to the child of the HOC despite being fetched.

  1. meteor create --react rmd-failure
  2. edit imports/ui/App.jsx to wrap Hello and Info components in React.StrictMode.
  3. Observe Learn Meteor links are not returned when Strict mode is enabled.
CaptainN commented 4 years ago

Props are definitely being passed to the child. I wonder what's up with that.

https://github.com/meteor/react-packages/blob/82bab2757f9e20ebe67143b389ecf44712508d43/packages/react-meteor-data/withTracker.jsx#L11

(Update: I misunderstood the problem based on the description - the props are passed fine, but the documents from the collection are getting lost in StrictMode)

CaptainN commented 4 years ago

I can confirm this is happening. I'll look at it as soon as I can find some time.

CaptainN commented 4 years ago

I took a look at this, and can confirm the behavior, but it's a little baffling. If I add a trace to the place inside withTracker where it passes the data to the component, I can see that reactivity it working all the way through. It just doesn't update the child component. I have no idea why not...

CaptainN commented 4 years ago

Even if I change it to use hooks, it still doesn't work. It does console log the data from within render - but doesn't actually update the dom. How strange.


export default Info = (props) => {
  console.log('render', props.links)

  const data = useTracker((c) => {
    console.log('reactive fn')
    const handle = Meteor.subscribe('links');
    console.log(handle.ready(), Links.find().fetch())
    return Links.find().fetch()
  })

  console.log(data)

  const links = data.map(
    link => makeLink(link)
  );

  return (
    <div>
      <h2>Learn Meteor!</h2>
      <ul>{ links }</ul>
    </div>
  );
}

function makeLink(link) {
  return (
    <li key={link._id}>
      <a href={link.url} target="_blank">{link.title}</a>
    </li>
  );
}
CaptainN commented 4 years ago

I still don't know why this is happening, but it only happens when no deps are supplied (which is always the case when using withTracker, since deps cannot be supplied). When I step through I can see everything coming back, and I can even trace through the last render, which does have the data - it just doesn't actually update the DOM for some reason. I have no idea if this would ever effect a real world scenario in production or if it's just some weird bug in Strict mode.

I can create a workaround, but it requires eating a second render for every mount. That's probably better than hard to catch failing edge cases, so I'll make a PR.

robraux commented 4 years ago

Since StrictMode is strictly non-production, it should never effect a production workload. Thanks for patching this @CaptainN -- I'll give it a whirl on our workload as soon as I'm able.

CaptainN commented 4 years ago

@robraux True, but StrictMode is meant to surface potentially difficult to spot problems. That doesn't mean this would be a real problem, but I figure better safe than sorry. This patch actually makes it follow the rules of hooks 100% for cases where no deps are supplied, so that's a bonus I guess.

At some point I'd like to refactor this entire thing to separate the deps implementation from the no-deps implementation. I think it'll make the code a lot easier to read and understand (and test). It's not important enough to stop the presses though.

robraux commented 4 years ago

@CaptainN I agree completely, hence filing the bug, but not being comfortable enough to patch it in a reasonable way.

We've used StrictMode to identify a number of lifecycle issues in development, so being able to reenable this will be wonderful.

CaptainN commented 4 years ago

I'm actually going to put it on my radar to do that refactor, and at the same time port it TypeScript! (And also to add a test for this bug.)