meteorrn / meteor-react-native

Meteor client for React Native matching Meteor Spec
https://guide.meteor.com/react-native.html
Other
59 stars 31 forks source link

withTracker no longer handles returned params being updated #54

Closed schlaegerz closed 3 years ago

schlaegerz commented 3 years ago

Describe the bug

With tracker no longer updates values passed in if you return it from the withTracker: I have a

export default withTracker(params=>{
    let myList = doStuffToList(params.list)
    return {
      myList: myList,
      status: Meteor.status(),
      user: Meteor.user(), 
  }
}) (ListContainer)

ListContainer never sees the myList change even though I can see withTracker being called again and having an updated value.

It was perfectly fine in 2.0.15 but not in the latest.

This is a break from the web package, as for me this is in a shared component and it only breaks when using the native package.

TheRealNate commented 3 years ago

Hi @schlaegerz, you're saying that you can see withTracker is recomputing and changing the myList value (via a console.log, etc), but the ListContainer never gets the updated value? Do you have any shouldComponentUpdate blocks in your ListContainer component? Could you also post the contents of doStuffToList if possible?

schlaegerz commented 3 years ago

Ok doStuffToList was just an example passing the list directly has the same issue.

So the problem is if list changes. withTracker is called again, but it doesn't re-render the component.

So if the parent component changes list from [] to ['a', 'b', 'c']

withTracker gets called with params.list equal to ['a', 'b', 'c']

but the child component never gets the update to rerender with ['a', 'b', 'c'] set to myList.

If I reload any other way it is fine (like a hot reload with a code change) and both web implementation and old versions of this package work perfectly fine

My guess is whatever is checking for updates in withTracker is only looking at Meteor values changing, and doesn't look at any props returned.

The work around for this particular case is just pass the myList param correctly, but thats not how I wrote all my code and it doesn't match the web implementation.

TheRealNate commented 3 years ago

Hi @schlaegerz,

Could you try adding a console log of params.list inside your withTracker function, that should tell you if withTracker is receiving the updates.

Could you also try adding a componentDidUpdate() to your ListContainer component to see if the props are being received there?

Thanks!

schlaegerz commented 3 years ago

That's exactly what I had done to see that this was a bug.

withTracker was getting called but componentDidUpdate() never did.

TheRealNate commented 3 years ago

Hi @schlaegerz,

Could you try using the latest beta version npm install --save @meteorrn/core@2.1.2-beta2? It includes an update to the whole Tracker framework (as our previous dependency hand't been updated in 5 years)

schlaegerz commented 3 years ago

I get No matching version found for @meteorrn/core@2.1.2-beta1

schlaegerz commented 3 years ago

Got the package but now I'm getting Unable to resolve module react-dom from node_modules/@meteorrn/core/src/Data.js: react-dom could not be found within the project or in these directories:

TheRealNate commented 3 years ago

Hey @schlaegerz, sorry about that. Could you try @meteorrn/core@2.1.2-rc1?

schlaegerz commented 3 years ago

Still happening on that build For maybe a reproducible version this is what I have


 withTracker(params=>{

    console.warn("LIST!", params.ListData)
    lastRet = {
      List: params.ListData,
      status: Meteor.status(),
      user: Meteor.user(), 
  }
    return lastRet
}) (ListContainer)

componentDidUpdate(prevProps:Props){
    if(this.props.List && this.props.List.length)
    {
      console.warn("UPDATE 1", this.props.List)
    }

     if(this.props.ListData && this.props.ListData.length)
    {
      console.warn("UPDATE 2", this.props.List)
    }

  }

The "LIST!" log gets called completely fine And I can see that this.props.ListData is getting updated but this.props.List is still an empty array.

If I trigger refresh it loads it just fine.

Obviously there is a fix for this case, but it worries me that this changed behavior and doesn't match normal meteor anymore.

TheRealNate commented 3 years ago

Hi @schlaegerz,

Thank you for the reproduction. While I am investigating this further, could you see if this issue occurs when you use useTracker?

Thanks!

iok97531 commented 3 years ago

Hi @TheRealNate I had same issue in 2.0.13, 2.1.0 but 2.0.15 works fine. I hope this information can help you

TheRealNate commented 3 years ago

Hi @iok97531 and @schlaegerz.

I was finally able to reproduce and confirm this issue. I have a temporary solution, which is using useTracker. Keep in mind that useTracker requires you to specify non-reactive deps (e.g. properties). For example:

import Meteor from '@meteorrn/core';

const MyComponent = props => {
    const {item} = Meteor.useTracker(() => {
        return {
            item:Items.findOne({_id:props.itemId})
        }
    }, [props.itemId]); // <- Note that we declare that the tracker depends on itemId
}

If you don't want to explicitly declare all dependencies, you can try this "new" withTracker, but there may be serious performance impacts. I haven't gotten a chance to fully test it yet.

import Meteor from '@meteorrn/core';

const withTrackerV2 = function (options) {
    return Component => {
        const expandedOptions = typeof options === 'function' ? { getMeteorData: options } : options;
        const { getMeteorData, pure = true } = expandedOptions;

        const WithTracker = forwardRef((props, ref) => {
            const data = Meteor.useTracker(() => getMeteorData(props) || {}, Object.values(props));
            return React.createElement(Component, {ref, ...props, ...data});
        });

        return pure ? memo(WithTracker) : WithTracker;
    };
}
TheRealNate commented 3 years ago

Hi @iok97531 and @schlaegerz,

I've started publishing very experimental builds under a new tag. Could you try installing the following version:

npm install --save @meteorrn/core@bleeding-edge

This release basically includes the changes mentioned above. Please note: there is the possibility of significant performance impacts, especially if you're passing a lot of props to the withTracker.

iok97531 commented 3 years ago

Hi @TheRealNate I installed with command

npm install --save @meteorrn/core@bleeding-edge

but I got this error

@meteorrn/core could not be found within the project or in these directories:
  node_modules/@meteorrn

And there is only node_modules folder in @meteorrn/core/

node_modules
  @meteorrn
    core
      node_modules

I think I need another folders like src or lib

TheRealNate commented 3 years ago

Hi @iok97531,

The command needs to be run from your root directory (where you'd run npm install when installing a new package). Sorry if that wasn't clear.

iok97531 commented 3 years ago

Hi @TheRealNate

I could not start app

I think Meteor dose not have _noYieldsAllowed function

See line number 97 in Tracker.js

TheRealNate commented 3 years ago

Hey @iok97531, sorry for the delay.

First: Please use this new install script so it will install directly from Git npm install --save git://github.com/TheRealNate/meteor-react-native.git#bleeding-edge

Second: I think that this new release may not solve your issue. There may be a deeper flaw with the way withTracker handles props, it may need a deeper redesign. However, one thing I have noticed is that useTracker seems to work well.

Could you try refactoring your code to use useTracker instead of withTracker and see if it works? Example:

Old:

const MyContainer = withTracker((props) => {
  // do stuff
  return {};
})(MyComponent);

New:

const MyContainer = props => {
  const data = useTracker(() => {
    // ...
  });

  return <MyComponent {...props} {...data}/>
};

Edit: useTracker also accepts a deps array thats passed on to useEffect. If this doesn't work, you can try using that:

const data = useTracker(() => {
  // ...
}, [dep1, dep2]);
rajtejani commented 3 years ago

Yes @TheRealNate using useTracker in place of withTracker is a better option for this situation. I was having the same problem where the component passed to withTracker function doesn't get updated results but replacing it with useTracker fix my problem.

TheRealNate commented 3 years ago

Hi @rajtejani and @iok97531, could you confirm that utilizing useTraker fixed the issue? I've been looking at the meteor spec and it seems that Meteor's withTracker re-exectues on any props change. I think our withTracker can be modeled similarly.

rajtejani commented 3 years ago

Yes @TheRealNate utilizing useTracker fixed the issue.

schlaegerz commented 3 years ago

Hmm yeah sorry I wasn't replying, useTracker does fix the issue, but I have a decent sized code based that is implemented with withTracker and its a code base shared with web so switching them all to useTracker doesn't seem like the right move for my code base.

Is this something that you would expect to be fixed in the near future (even just going back to the 2.0.15 implementation) or should I be looking at refactoring/other work arounds?

TheRealNate commented 3 years ago

@schlaegerz does 2.0.15 solve the issue for you? If so I can absolutely revert withTracker to that version.

schlaegerz commented 3 years ago

Yep 2.0.15 works just fine, and matches with the normal meteor implementation

TheRealNate commented 3 years ago

Hey @schlaegerz,

Version 2.2.0 has been released, and it contains a fix that I believe should solve your issue.

Could you try it? You can use npm update or npm install --save @meteorrn/core@2.2.0

Thanks!

TheRealNate commented 3 years ago

Hey @rajtejani,

Since you were also experiencing this issue: version 2.2.0 has been released and it should fix the issue with withTracker. Could you try it out?

Thanks!

schlaegerz commented 3 years ago

Yeah that was my change that was pulled in, This issue is fixed