Skip to content

Hooks - #3

Merged
menelike merged 5 commits into
risetechnologies:hooksfrom
CaptainN:hooks
Jul 5, 2019
Merged

Hooks#3
menelike merged 5 commits into
risetechnologies:hooksfrom
CaptainN:hooks

Conversation

@CaptainN

@CaptainN CaptainN commented Jul 4, 2019

Copy link
Copy Markdown
  • Condenses refs into 1 useRef.
  • Avoids running reactiveFn in asynchronous block, if it will run synchronously next render anyway.

@CaptainN

CaptainN commented Jul 4, 2019

Copy link
Copy Markdown
Author

This addresses the last concern mentioned in #1 - if you like the idea of passing in the computation to the reactiveFn mentioned in #2, I can add that to this PR too.

@menelike

menelike commented Jul 4, 2019

Copy link
Copy Markdown
Member

Nice!

Any reason you did not include the c.stop() like described in here: #1 (comment)

if you like the idea of passing in the computation to the reactiveFn mentioned in #2, I can add that to this PR too.

I'd like to track that in #2 , let's first focus on fixing the goal of this PR.

// Only run reactiveFn if the hooks have not change, or are not falsy.
if (areHookInputsEqual(deps, refs.previousDeps)) {
runReactiveFn();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else { c.stop() }

IMHO the computation should be stopped as fast as you can to avoid async related issues.
Who knows when react decides to update the component which then calls dispose().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That makes sense - I also wonder if areHookInputsEqual(deps, refs.previousDeps) is overkill - the only real case I think we need to check for here is if either deps or refs.previousDeps are falsy, because the enclosed value of dep and the refs value should never really change between the last run (which did an equality check) and the reactive function execution. In other words, they will always be made equal before the computation runs, because if deps changes, the computation is stopped/restarted and those values made equal at the same time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, if deps and prevDeps are different, they are always handled within react's lifecycles.
I don't see any reason why !deps || !refs.previousDeps should work.

@menelike

menelike commented Jul 4, 2019

Copy link
Copy Markdown
Member

The code looks good, I'll make some in-depth tests by tomorrow, I don't want to risk a third PR for this.

@DevelopmentByDavid

Copy link
Copy Markdown

Hey, this all looks great! If you guys need any help I'd be happy to if I can.

@menelike

menelike commented Jul 5, 2019

Copy link
Copy Markdown
Member

@CaptainN

This PR also solves the issue I had with the increased number of subscriptions as explained in
#1 (comment)

Everything seems to work!

@menelike
menelike merged commit d801348 into risetechnologies:hooks Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants