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

Consistency of hook dependencies #463

Closed
sgrishchenko opened this issue May 31, 2021 · 7 comments · Fixed by #470 or #476
Closed

Consistency of hook dependencies #463

sgrishchenko opened this issue May 31, 2021 · 7 comments · Fixed by #470 or #476

Comments

@sgrishchenko
Copy link
Collaborator

sgrishchenko commented May 31, 2021

Now there are three ways to use dependencies of hooks:

  1. Array (dependencies are last): useCallback({ value }, arrayOf(value))
  2. Vararg (dependencies are first): useCallback(value) { value }
  3. List (dependencies are first): useEffect(listOf(value)) { println(value) }

I suggest to use only one form - Vararg (dependencies are first). I think it is most compact form and Kotlin friendly way (because callbacks are last parameter in this case).

I can create PR, where I will add overloads for this kind of API and mark all other form as deprecated for backward compatibility.

@Leonya
Copy link
Collaborator

Leonya commented May 31, 2021

@Skolotsky @turansky WDYT?

@Skolotsky
Copy link
Contributor

Skolotsky commented May 31, 2021

i like vararg version

@turansky
Copy link
Collaborator

turansky commented May 31, 2021

useEffect + useLayoutEffect - last non-updated hooks

I use/like vararg version only :)

inline fun useEffectOnce(
    noinline effect: () -> RCleanup,
) {
    rawUseEffect(effect, emptyArray())
}

inline fun useEffect(
    vararg dependencies: dynamic,
    noinline effect: () -> RCleanup,
) {
    rawUseEffect(effect, dependencies)
}

// super draft name
inline fun useSimpleEffect(
    vararg dependencies: dynamic,
    noinline effect: () -> Unit,
) {
    rawUseEffect(effect, dependencies)
}

Sync with useLayoutEffect - open question :(

cc @vlsi

@turansky
Copy link
Collaborator

turansky commented May 31, 2021

Also EffectBuilder can be used to minimize methods count:

var count by useState(3)

useEffect(count) {
    val timerId = window.setTimeout({ count += 1 }, 2000)

    cleanup { window.clearTimeout(timerId) }
}

With effect builder we will have:

  1. useEffect() - every render
  2. useEffectOnce() - one time (from react-use)
  3. useEffect(a, b, c) - on change

Without effect builder we will have x2 methods (with ugly withCleanup) :(

@Leonya
Copy link
Collaborator

Leonya commented May 31, 2021

@sgrishchenko please create a PR

@sgrishchenko
Copy link
Collaborator Author

sgrishchenko commented Jun 4, 2021

To be honest, I have initiated this issue because now there is not any mechanism to protect user from missing some dependencies in hooks like useEffect or useMemo. There is linter plugin for it In JS world, but I am planing to write IDE Inspection for Kontlin that makes the same thing. But it is difficult to recognize right and wrong patterns of hooks using if there aren't some conventions, that is why I have created this issue.

So, I have thought more and better about hooks API and I have changed my mind: now I am proposing to use Arrays instead Varargs (Array is first, Callback is last):

Pros of Arrays:

  1. It is easier to recognize boundaries of dependencies for custom hooks. For example we have such custom hook:
    fun useCustomHook(firstArg: String, vararg dependencies: dynamic, callback: () -> Unit)
    
    And here is usage of this hook
    useCustomHook(firstArg, firstDep, secondDep) { ... }
    
    As you can see, it is difficult to determine where is deps and where is additional args and I will have to use signature of function instead of call expression. Situation becomes more complicated if I have overloads because dependencies are dynamic:
    fun useCustomHook(vararg dependencies: dynamic, callback: () -> Unit)
    fun useCustomHook(firstArg: String, vararg dependencies: dynamic, callback: () -> Unit)
    fun useCustomHook(firstArg: String, secondArg: Int, vararg dependencies: dynamic, callback: () -> Unit)
    
    So now I have to use some algorithm to determine what overload will be used, so if dependencies are separated by special construction it is much easier:
    useCustomHook(firstArg, arrayOf(firstDep, secondDep)) { ... }
    
  2. I think if Arrays are used for hooks API it will be more friendly for people, who will switch from JS, because concepts what were in JS hooks will almost be the same in Kotlin (e.g. React team could use spread operator for hooks but they did not)
  3. It is not necessary to introduce additional useEffectOnce hook to make difference between one time effect and effect what runs on each update (it is good point for new users because they just can use concept from official documentation about hooks).

Cons of Arrays:

  1. Syntax will be a little bit more verbose, but I think it is not a big deal because some of principles of Kotlin are Readability and Pragmatism so if I want I can write:
useCustomHook(firstArg, dependencies = arrayOf(firstDep, secondDep)) { ... }

And now everyone can see than it is not just random data, it is dependencies, and, it seems I can't do the same with Vararg.

@sgrishchenko
Copy link
Collaborator Author

I have prepared PR with implementation of my suggestions and I have added EffectBuilder, thanks for @turansky, it looks really cool. I also added KDocs with examples and deprecation annotations with replacements (where it is possible to generate code without compile errors) and suggestions how to migrate to new API. See #468

This was referenced Jun 4, 2021
@Leonya Leonya reopened this Jun 5, 2021
sgrishchenko added a commit to sgrishchenko/kotlin-wrappers that referenced this issue Jun 7, 2021
Formatting is improved
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 a pull request may close this issue.

4 participants