[Frame] hookify Loading #2303

sijad · 2019-10-16T20:05:05Z

WHY are these changes introduced?

related to #1995

WHAT is this pull request doing?

change Frame.Loading to use react hooks

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

sijad · 2019-10-16T21:04:57Z

I'm not sure how tests can improved?

sijad · 2019-10-16T21:25:41Z

src/components/Frame/components/Loading/Loading.tsx

-    });
-  }
+  const ariaValuenow = useLazyRef(() =>
+    debounce((progress: number) => {


In this aproach component will only re-rendered when progress changes, unlike the old aproach which would re-render with each requestAnimationFrame call. I guess it'd be safe to remove debounce and calculate the valenow inline.

This is a a pure function that doesn't depend on anything in the outer closure, so there's no need to put it inside the component or do any of the lazyRef stuff, just define it as a regular function outside of the component function

That said, now that we're only rendering this once per progress change I'm good with just defining this value inline. If I've understood your other changes - where progress is now a number between 0 and 1, rounded to two decimal places this should do:

aria-valuenow={progress * 100}

I'll refactor it and remove debounce, just FTR if we move that function outer closure, multiple instance of this component will use same debounced ref and it might cause data leak between those instance (they mill use each other values).

BPScott · 2019-10-17T00:29:07Z

src/components/Frame/components/Loading/Loading.tsx

-      </div>
-    );
-  }
+      animation = requestAnimationFrame(increment);


Setting the value of a variable outside of the current scope is surpising and we should try and avoid such mutation if possible. Instead of setting the value of an variable outside the current scope, return the value from requestAnimationFrame

Suggested change

animation = requestAnimationFrame(increment);

return requestAnimationFrame(increment);

Then below set the value of animation

const animation = increment();

we should track latest animation for calling cancelAnimationFrame in un-mounting, I'm not sure if const animation = increment(); will works.
should we use ref?

Just chiming in const animation = increment(); won't keep track of the recursed animation as the assignment will

src/components/Frame/components/Loading/Loading.tsx

BPScott · 2019-10-17T01:15:43Z

src/components/Frame/components/Loading/Loading.tsx

-    });
-  }
+  const ariaValuenow = useLazyRef(() =>
+    debounce((progress: number) => {


This is a a pure function that doesn't depend on anything in the outer closure, so there's no need to put it inside the component or do any of the lazyRef stuff, just define it as a regular function outside of the component function

That said, now that we're only rendering this once per progress change I'm good with just defining this value inline. If I've understood your other changes - where progress is now a number between 0 and 1, rounded to two decimal places this should do:

aria-valuenow={progress * 100}

src/components/Frame/components/Loading/Loading.tsx

BPScott · 2019-10-17T01:43:03Z

src/components/Frame/components/Loading/Loading.tsx

-    }
-  }
+    const increment = () => {
+      if (currentProgress >= STUCK_THRESHOLD) {


You seem to have changed the progress from being an integer between 0 an 100, to a number rounded to 2 decimal places between 0 and 1. I'm ok with that, but make sure that you update the stuck threshold and anything else like how the next step is calculated to account for that.

currentProgress still go from 0 - 100 with not limited decimal point (e.g. 20, 21.332232332, 22.12321, 23.5565, etc)
nextProgress and previousProgress goes from 0 - 1 with only two decimal point (e.g. 0.12, 0.13, 0.14, 0.15)
with this approach set state called only when nextProgress and previousProgress are not equal which save redundant re-rendering. if we set currentProgress in state, it'd re-render more often (due its decimals changes with each requestAnimationFrame call)

BPScott · 2019-10-17T01:50:48Z

src/components/Frame/components/Loading/Loading.tsx

-  componentWillUnmount() {
-    const {animation} = this.state;
+  useEffect(() => {
+    let animation: number | undefined;


Should animation, step currentProgress and previousProgress be refs? It seems to work ok in this closure but I'm not sure why.

as we don't need those outside of useEffect I moved them all there, a race condition might occurs?

Should animation, step currentProgress and previousProgress be refs? It seems to work ok in this closure but I'm not sure why.

From the current implementation, it should work as-is from what I can tell. We're creating a closure, recursing and mutating the original values. setProgress will be consistently called with the correct values, and when the component is dismounted it'll have the current animation in scope to clear.

AndrewMusgrave · 2019-11-20T23:31:54Z

Sorry for the wait @sijad @BPScott is busy at the moment and won't be able to review this again. If it's ready for another review let me know and I can take look!

sijad · 2019-11-21T08:46:18Z

no worries, take your time
there're few unresolved conversations but it'd be great if you could review this again

src/components/Frame/components/Loading/Loading.tsx

AndrewMusgrave · 2019-11-21T22:16:10Z

src/components/Frame/components/Loading/Loading.tsx

-  componentWillUnmount() {
-    const {animation} = this.state;
+  useEffect(() => {
+    let animation: number | undefined;


Should animation, step currentProgress and previousProgress be refs? It seems to work ok in this closure but I'm not sure why.

From the current implementation, it should work as-is from what I can tell. We're creating a closure, recursing and mutating the original values. setProgress will be consistently called with the correct values, and when the component is dismounted it'll have the current animation in scope to clear.

AndrewMusgrave · 2019-11-21T22:19:32Z

src/components/Frame/components/Loading/Loading.tsx

-      </div>
-    );
-  }
+      animation = requestAnimationFrame(increment);


Just chiming in const animation = increment(); won't keep track of the recursed animation as the assignment will

sijad · 2019-11-23T10:10:39Z

let me know if there's anything else than can be improved.
I'd also work on tests but as this component uses requestAnimationFrame I couldn't figure it out how?

AndrewMusgrave · 2019-11-26T23:57:22Z

Mocking can be done a bunch of ways in jest, however, for RAF you'll probably want to go with one of these two depending on what you're trying to do.

describe('suite', () => {
  let requestAnimationFrameSpy: jest.SpyInstance;

  beforeEach(() => {
    requestAnimationFrameSpy = jest.spyOn(window, 'requestAnimationFrame');
    requestAnimationFrameSpy.mockImplementation((cb) => {
      cb();
      // unique id used to cancel rafs
      return 1;
    });
  });

  afterEach(() => {
    requestAnimationFrameSpy.mockRestore();
  });

  ...
  it statements
  ...
})

or

const origRequestAnimationFrame = window.requestAnimationFrame;
describe('suite', () => {
  afterEach(() => {
    // This might require Object.defineProperty
    window.requestAnimationFrame = origRequestAnimationFrame;
  });

  ...
  it('statement', () => {
    // This might require Object.defineProperty
    window.requestAnimationFrame = someMockRaf;
  })
  ...
})

// Object.defineProperty usage
// Object.defineProperty(window, 'requestAnimationFrame', {
//   configurable: true,
//   writable: true,
//   value: someMockFn,
// })

If you have any specific questions I'm happy to help :)

sijad · 2019-11-27T20:42:37Z

@AndrewMusgrave thanks for the tips, I get Maximum call stack size exceeded error with recursive requestAnimationFrameSpy method, I went and used animationFrame

sijad · 2019-11-27T21:24:16Z

src/components/Frame/components/Loading/tests/Loading.test.tsx


-  it('mounts', () => {
-    expect(loading.exists()).toBe(true);
+    for (let i = 0; i <= 100; i++) {


is this a good approach?
we can also change 100 to 100,000 to have a 100% code coverage but it took ~7 seconds in my computer...

ghost · 2020-05-25T22:05:31Z

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

sijad · 2020-06-02T11:26:02Z

this is very disappointing, at first I thought this is a community driven project, but now I realized I was wrong.

sijad force-pushed the hookify-frame-loading branch 4 times, most recently from 90e8085 to 72ced88 Compare October 16, 2019 20:48

sijad commented Oct 16, 2019

View reviewed changes

BPScott reviewed Oct 17, 2019

View reviewed changes

sijad force-pushed the hookify-frame-loading branch from 72ced88 to 64e93ee Compare October 17, 2019 16:55

AndrewMusgrave self-requested a review November 21, 2019 20:44

AndrewMusgrave reviewed Nov 21, 2019

View reviewed changes

sijad force-pushed the hookify-frame-loading branch from 64e93ee to a889964 Compare November 23, 2019 09:59

sijad force-pushed the hookify-frame-loading branch 3 times, most recently from 69de5ec to e05f0c2 Compare November 27, 2019 19:31

sijad force-pushed the hookify-frame-loading branch 3 times, most recently from 9c66eab to a06acac Compare November 27, 2019 20:54

sijad added 2 commits November 28, 2019 00:33

hookify Frame Loading

1ca4ab9

add changelog entry

8ca6c15

sijad force-pushed the hookify-frame-loading branch from a06acac to 6e85a5f Compare November 27, 2019 21:04

add more tests

d8ba2fc

sijad force-pushed the hookify-frame-loading branch from 6e85a5f to d8ba2fc Compare November 27, 2019 21:16

sijad commented Nov 27, 2019

View reviewed changes

ghost added the Icebox label May 25, 2020

ghost closed this Jun 1, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frame] hookify Loading #2303

[Frame] hookify Loading #2303

sijad commented Oct 16, 2019

sijad commented Oct 16, 2019

sijad Oct 16, 2019

BPScott Oct 17, 2019

sijad Oct 17, 2019

BPScott Oct 17, 2019

sijad Oct 17, 2019

AndrewMusgrave Nov 21, 2019

BPScott Oct 17, 2019

BPScott Oct 17, 2019

sijad Oct 17, 2019

BPScott Oct 17, 2019

sijad Oct 17, 2019

AndrewMusgrave Nov 21, 2019

AndrewMusgrave commented Nov 20, 2019

sijad commented Nov 21, 2019 •

edited

Loading

AndrewMusgrave Nov 21, 2019

AndrewMusgrave Nov 21, 2019

sijad commented Nov 23, 2019 •

edited

Loading

AndrewMusgrave commented Nov 26, 2019

sijad commented Nov 27, 2019

sijad Nov 27, 2019

ghost commented May 25, 2020

sijad commented Jun 2, 2020

	animation = requestAnimationFrame(increment);
	return requestAnimationFrame(increment);

[Frame] hookify Loading #2303

[Frame] hookify Loading #2303

Conversation

sijad commented Oct 16, 2019

WHY are these changes introduced?

WHAT is this pull request doing?

🎩 checklist

sijad commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewMusgrave commented Nov 20, 2019

sijad commented Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijad commented Nov 23, 2019 • edited Loading

AndrewMusgrave commented Nov 26, 2019

sijad commented Nov 27, 2019

Choose a reason for hiding this comment

ghost commented May 25, 2020

sijad commented Jun 2, 2020

sijad commented Nov 21, 2019 •

edited

Loading

sijad commented Nov 23, 2019 •

edited

Loading