Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize result object #26

Closed
wants to merge 1 commit into from

Conversation

fabiosantoscode
Copy link

This makes it possible to use the return value from the hooks in this library
as dependencies for other hooks.

Closes #17

This makes it possible to use the return value from the hooks in this library
as dependencies for other hooks.
fetchMore: fetchMoreAsync.execute,
isEnd,
}),
[canFetchMore, fetchMoreAsync, isEnd]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchMoreAsync is memoized so no need to check for its individual methods.

currentPromise,
currentParams,
]
);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that AsyncState.value is never mutated, so the memo dependency is just .value itself.

@fabiosantoscode
Copy link
Author

In #17 there's talk of memoizing the execute callback itself but I'm not confident I understand the codebase enough to do this.

@slorber
Copy link
Owner

slorber commented Jun 9, 2020

Hey @fabiosantoscode , thanks for this PR

Unfortunately that does not look like something I'd like to merge. I don't think it's very important to guarantee to the user that the return value is memoized, as it does not look to me a good practice to use such big object as a dependency in the first place. You'd rather add to the dependency array individual properties you actually need in the effect (for example [asyncResult.status, asyncResult.execute].

To me the current problem this library has is:

  • In current version: the execute function is declared inline, this is the first thing we should focus on memoizing because it prevents users to use it in a dependency array:

    const executeAsyncOperation = (...args: Args): Promise<R> => {

  • In V2: The api should probably return something like [state, api] instead of a big object. This is a breaking change but this would allow to ensure that at least the api part is always the same object, allowing you to use [api] as dependency.


As execute is declared inline, your fix wouldn't work because the memo would receive a different execute fn instance at each re-render.

@fabiosantoscode
Copy link
Author

You're completely right that execute should be more stable.

However I'm not sure how to fix this, so it's probably best to close.

@slorber
Copy link
Owner

slorber commented Jun 11, 2020

ok, I'm going to handle everything myself when I have some time :)

@slorber slorber closed this Jun 11, 2020
@henrymoulton
Copy link

henrymoulton commented Jul 13, 2021

@slorber just checking here - pretty sure I ran into this issue, this is still an issue right?

@slorber
Copy link
Owner

slorber commented Sep 24, 2021

Still an issue yes and I don't think it's worth stabilizing the result object, because it will change anyway.

You'd better use each returned attribute independently.

I just made the "execute()" callback stable so you can use it in deps arrays, other callbacks were already stable

In the future, I'd like to move to [state, api] where api remains stable and could be used in deps array as a whole

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memoize useAsyncCallback ?
3 participants