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

Add useMemo, tests, and prettier (unformatted) #11

Merged
merged 22 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@tannerlinsley
Copy link
Contributor

tannerlinsley commented Jan 1, 2019

No description provided.

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 1, 2019

Took some git selection foo, but this should do the trick. If, and when, you're ready you can run yarn format to adopt prettier styling to the repo.

@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 1, 2019

Thanks!

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 1, 2019

I think it's worth noting that the useCallback hook can now also be implemented using the useMemo hook:

function useCallback (fn, ...guards) {
	return useMemo(() => fn, ...guards)
}
@getify
Copy link
Owner

getify left a comment

Overall, this is a good start at the feature. Thanks!

But, I think this implementation diverges from how React does useMemo(..). React's version of the hook returns a self-contained memoized function instead of just invoking the function right away and returning its value. We want to stick as close as reasonable to React's versions of these hooks.

The way to implement this seems to be that we should initially store the user-provided function in memoization[0] slot, then only once the memoized function is invoked, call the original function stored in that slot, replace its return value there, and also return that value from the memoized function. From there on out, the memoized function should just keep returning the value in the memoization[0] slot.

( Sorry, the above was my confusion (with the useCallback(..) hook, not useMemo(..)). )


However... there is something missing from the current implementation.

https://reactjs.org/docs/hooks-reference.html#usememo

If no array is provided, a new value will be computed whenever a new function instance is passed as the first argument. (With an inline function, on every render.)

It seems like if no guards list is passed, you have to put a reference to the original function as the only value in the guards list, and make the guards comparison accordingly each time.

Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved tests/tests.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Show resolved Hide resolved src/tng-hooks.src.js Outdated
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 2, 2019

Can you point me to the react hooks source you're referring to?

@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 2, 2019

Oh, oops. Sorry, I think I got confused with the useCallback(..) hook, which does indeed return a new wrapped function, not the result of calling the provided function. IIUC, your suggested implementation above for useCallback(..) wouldn't be correct, because your version calls useMemo(..) right away, it doesn't return a wrapped function.


However, the docs here do suggest a bit of behavior for useMemo(..) that I don't think is currently handled by your implementation:

https://reactjs.org/docs/hooks-reference.html#usememo

If no array is provided, a new value will be computed whenever a new function instance is passed as the first argument. (With an inline function, on every render.)

IOW, it seems like if no guards list is passed, you have to put a reference to the original function as the only value in the guards list, and make the guards comparison accordingly each time.

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 2, 2019

Yeah I can see that in the react hooks source now. Simple enough. My original comment on how to implement useCallback is still valid. Both hooks use the same mechanism to determine when a new value is returned. The only difference is that useCallback returns the function you pass as the first argument, instead of executing and returning its result. Clearly you could duplicate the useMemo code and make that single change, but I say why waste?

@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 2, 2019

Thanks, I got it now. Sorry for my confusion on that stuff.

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 2, 2019

No worries. Hooks are still very fresh, so it's easy to lose track of what does what and how. Most of my understanding came from writing my use-react-hooks packport for older react versions.

getify and others added some commits Jan 2, 2019

Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update tests/tests.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update src/tng-hooks.src.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
Update tests/tests.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>

tannerlinsley and others added some commits Jan 2, 2019

Update tests/tests.js
Co-Authored-By: tannerlinsley <tannerlinsley@gmail.com>
@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 7, 2019

Just wanted to check, would you like to make the suggested updates here, or would you like me to pull this into a fork and do them before merging?

Sorry, hadn't seen your recent updates. Looks good. I'll pull this in soon. Appreciate the contribution!

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 7, 2019

👍

@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 7, 2019

@tannerlinsley Can I ask one more thing of you? Can you squash all the commits into a single one (I guess maybe with a new PR), so that when I pull that into a branch to fix up, it has a single commit attributed to you? Otherwise, I think it's going to erase your attribution.

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 7, 2019

If you go into your repo settings and enable the sqaush merge option for PR's, then all you need to do is select that option on this PR and click merge. All attribution will be retained.

@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 8, 2019

OK, cool, that can work.

I was going to pull this into a separate branch first, do any extra fixes to tests and some extra documentation there, and then merge back to master. The squash merge will go directly to master, though. I wonder if there's a way to redirect to a separate branch? Yep, done. Thanks!

@getify getify self-assigned this Jan 8, 2019

@getify

getify approved these changes Jan 8, 2019

@getify getify changed the base branch from master to feature-useMemo Jan 8, 2019

@getify getify merged commit 71c64b5 into getify:feature-useMemo Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@getify

This comment has been minimized.

Copy link
Owner

getify commented Jan 8, 2019

Merged useMemo(..) into master just now (after cleaning up tests and docs). will release it as 4.1.0, but after I add the useRef(..) hook.

Thanks again!

@tannerlinsley

This comment has been minimized.

Copy link
Contributor

tannerlinsley commented Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment