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

**Feature:** Splash hooks & useInterval #940

Merged
merged 22 commits into from
Mar 11, 2019

Conversation

JoshRosenstein
Copy link
Contributor

@JoshRosenstein JoshRosenstein commented Feb 27, 2019

Summary

Convert Splash component to use hooks and optimized a bit.

  • Removed State from main Splash Component( window state moved to Splash.Animation)

  • Added hook useInterval (forked from use-interval)

  • Updated Splash.Animation to use hooks useWindowSize & useInterval
    -useInterval has option for immediate, therefore animation is triggered immediately when loaded.

  • Updated small logic where dynamic styles (top & left) do not pass through styled to eliminate hectic class generation

  • Added fullscreen prop inorder to trigger useWindowSize

  • use type React.FC ( SFC is depreciated)

  • Fixed type warning on intialState array creator Array.apply(null, { length: boxes }) .map(Number.call, Number) to Array.apply(null, Array(boxes)).map((_: any, i: number) => i)

Related issue

#910

To be tested

Me

  • No error or warning in the console on localhost:6060

@@ -0,0 +1,37 @@
import { useEffect, useRef } from "react"
Copy link
Contributor

@stereobooster stereobooster Feb 28, 2019

Choose a reason for hiding this comment

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

The simplest implementation would be

import { useEffect } from 'react';

const useInterval = (fn: () => void, delay: number) => {
  useEffect(() => {
    const id = setInterval(fn, delay)
    return () => clearInterval(id)
  })
};

export default useInterval;

UPD code works only because useEffect will be triggered on each render (which is wrong)

  useEffect(() => {
    const id = setInterval(fn, delay)
    return () => clearInterval(id)
-  })
+  }, [delay])

if we would add delay, it would brake. Example of the bug

const useInterval = (fn: () => void, delay: number) => {
  useEffect(() => {
    const id = setInterval(() => {
      console.log(1);
      fn();
    }, delay)
    return () => clearInterval(id)
  }, [delay])
};

function Counter() {
  const [count, setCount] = useState(0);
  useInterval(() => {
    // Your custom logic here
    setCount(count + 1);
  }, 1000);
  return <h1>{count}</h1>;
}

count will change from 0 to 1 only once (because count is locally bound to function passed to useInterval), but console.log(1); will be called many times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stereobooster ,Dan wrote a nice piece on this.

Simple Implementation works for simple cases ( and how its currently being used in Splash)

But this version is more scalable/reusable ...

"But setInterval() does not “forget”. It will forever reference the old props and state until you replace it — which you can’t do without resetting the time."

Where useRef comes into play...

"The problem boils down to this:

  • We do setInterval(callback1, delay) with callback1 from first render.
  • We have callback2 from next render that closes over fresh props and state.
  • But we can’t replace an already existing interval without resetting the time!

So what if we didn’t replace the interval at all, and instead introduced a mutable savedCallback variable pointing to the latest interval callback?"

"This mutable savedCallback needs to “persist” across the re-renders. So it can’t be a regular variable. We want something more like an instance field."

For Example, this allows us to change the delay or callback between renders without resetting the time

But if you want be to change to simple version, i will do that!

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that the code should be clear, so people after us would be able to read it easily and change it if they want to. It takes quite some iterations to understand what happens here. After reading Dan's article this all start to make sense, but before it seemed over-engineered. So adding link to the Dan's article and maybe some more comments would help

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. @JoshRosenstein please add some documentation if you wish to keep the component the way it is. ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshRosenstein have you seen these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpotomin - It was my understanding the "simplest implementation" version breaks, and should keep the current useInterval as is, and i added additional comments since, Did i miss something ? Or just forgot to resolve?

Edit useInterval Test

Copy link
Contributor

Choose a reason for hiding this comment

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

You just forgot to acknowledge. Thanks for responding, @JoshRosenstein! @stereobooster is that okay with you?

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

@JoshRosenstein this is incredible as the others are 😍

Let's adjust some stuff and 🚀

src/Splash/Splash.Animation.tsx Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Outdated Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Outdated Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Outdated Show resolved Hide resolved
src/Splash/Splash.tsx Outdated Show resolved Hide resolved
src/useInterval/index.ts Outdated Show resolved Hide resolved
src/useInterval/index.ts Show resolved Hide resolved
src/useInterval/useInterval.test.tsx Outdated Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Show resolved Hide resolved
@@ -0,0 +1,37 @@
import { useEffect, useRef } from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. @JoshRosenstein please add some documentation if you wish to keep the component the way it is. ❤️

src/useInterval/index.ts Show resolved Hide resolved
Copy link
Contributor

@mpotomin mpotomin left a comment

Choose a reason for hiding this comment

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

Looks great, @JoshRosenstein! Let's change some of these little things and get this merged! 🚀

src/Splash/Splash.Animation.tsx Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Show resolved Hide resolved
src/Splash/Splash.Animation.tsx Outdated Show resolved Hide resolved
src/useInterval/README.md Outdated Show resolved Hide resolved
src/useInterval/README.md Outdated Show resolved Hide resolved
src/useInterval/README.md Outdated Show resolved Hide resolved
src/useInterval/README.md Show resolved Hide resolved
src/useInterval/README.md Outdated Show resolved Hide resolved
src/useInterval/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,37 @@
import { useEffect, useRef } from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshRosenstein have you seen these comments?

mpotomin and others added 4 commits March 4, 2019 11:49
Co-Authored-By: JoshRosenstein <32781407+JoshRosenstein@users.noreply.github.com>
Co-Authored-By: JoshRosenstein <32781407+JoshRosenstein@users.noreply.github.com>
Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

We're just finishing up the day here in Berlin, @JoshRosenstein, but I'll be sure to look at this with fresh eyes 👀 in the morning and get back to you.

Thanks for your amazing work so far! You've already got quite a few PRs merged! 🎉 You're on fire! 🔥

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Looking good, @JoshRosenstein!

src/useInterval/index.ts Show resolved Hide resolved
Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Found a copy/paste to remove.

src/useInterval/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Looks good to me!

src/Splash/Splash.Animation.tsx Outdated Show resolved Hide resolved
src/useInterval/useInterval.test.tsx Outdated Show resolved Hide resolved
@TejasQ TejasQ requested a review from mpotomin March 6, 2019 16:10
Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Ouch. Almost.

image

Tejas Kumar and others added 2 commits March 6, 2019 23:07
Co-Authored-By: JoshRosenstein <32781407+JoshRosenstein@users.noreply.github.com>
Co-Authored-By: JoshRosenstein <32781407+JoshRosenstein@users.noreply.github.com>
Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Unfortunately, this is still broken on the demo.

image

@TejasQ TejasQ dismissed mpotomin’s stale review March 11, 2019 14:26

Tejas was wrong

@TejasQ TejasQ merged commit a912bc2 into contiamo:master Mar 11, 2019
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.

4 participants