Fix: debounce fetch apps by nicosampler · Pull Request #1021 · 5afe/safe-react

nicosampler · 2020-06-16T16:44:46Z

closes #999.

ghost · 2020-06-16T17:26:14Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

src/routes/safe/components/Apps/ManageApps.tsx

mmv08 · 2020-06-18T10:08:55Z

src/routes/safe/components/Apps/utils.ts

 ]

-export const getAppInfoFromOrigin = (origin) => {
+export const getAppInfoFromOrigin = (origin: string): Record<string, any> | null => {


please change any to unknown

I did, but it was complaining about the "unknown" type, I'll try again.

Any updates of this ? @fernandomg

shame on me that assumed those were all old. I'll have a look

mmv08 · 2020-07-07T12:03:03Z

@nicosampler what's up with this ticket?

mmv08 · 2020-07-16T12:37:48Z

@nicosampler are you going to finish this or somebody needs to take this?

nicosampler · 2020-07-16T12:43:43Z

I've been working on it, and I got stuck working with the debounce function, for some reason (that could not discover yet), the function is being re-created due to a re-render all the time and that force the requests to be triggered all the time.

Feel free to take a look, I'm working with TXs now.

mmv08 · 2020-07-16T12:45:38Z

I got stuck working with the debounce function, for some reason

In that case you should ask for help, we can't help if we don't know you need it

nicosampler · 2020-07-16T13:45:48Z

I did, worked on it with Fernando, then I had to move to another ticket.

mmv08 · 2020-07-16T13:49:32Z

I did, worked on it with Fernando, then I had to move to another ticket.

Working silently with someone on a fix != sharing a problem with a team so anyone can take a look at it

ghost · 2020-07-22T18:42:30Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

I'm planning to remove this dependency, as it requires to also install `lodash.debounce`. I prefer to implement it ad-hoc.

…n over and over

github-actions · 2020-07-23T16:07:02Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	2	0
Ignored	0	N/A

Result: ✅ success
Annotations: 2 total

[warning] @typescript-eslint/no-non-null-assertion

Disallows non-null assertions using the ! postfix operator

src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 74 - Forbidden non-null assertion.
src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 74 - Forbidden non-null assertion.

^{Report generated by eslint-plus-action}

ghost · 2020-07-23T16:18:24Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

ghost · 2020-07-24T12:18:13Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

francovenica · 2020-07-24T19:30:58Z

I still get a chunk of error by writing an ULR. Is not as aggressive like it is in prod where there like 5 error per character you type, but there they are. I don't know how much we can avoid it, so this is just how I see it in this PR right now

I've tried to type this URL https://ipfs.io/ipfs/QmTgnb1J9FDR9gimptzvaEiNa25s92iQy37GyqYfwZw8Aj/

fernandomg · 2020-07-24T19:56:51Z

I still get a chunk of error by writing an ULR. Is not as aggressive like it is in prod where there like 5 error per character you type, but there they are. I don't know how much we can avoid it, so this is just how I see it in this PR right now

I've tried to type this URL ipfs.io/ipfs/QmTgnb1J9FDR9gimptzvaEiNa25s92iQy37GyqYfwZw8Aj

Well, we can prevent logging the errors. But sometimes they're useful.

The thing with the number of errors has to do with how fast you type if you take more than 500ms between keystrokes, then you'll have an error per char.

Said so, we can increase the 500ms delay, or set it as an ENV var so it's easier to tweak it when needed? Not sure. @mikheevm, @nicosampler

also, moved `initialValues` to a constant `INITIAL_VALUES` outside the component

also fixed types

nicosampler · 2020-07-24T20:29:31Z

It's totally fine log these errors, a partial URL is not a valid safeApp. Regarding the 500ms, I think it's the standard.

mmv08 · 2020-07-24T20:38:46Z

Yeah I agree that logging these errors is perfectly fine.

ghost · 2020-07-24T20:53:38Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

ghost · 2020-07-24T21:02:10Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

francovenica · 2020-07-27T13:39:11Z

Gotcha. Then It's fine for me

mmv08 · 2020-07-27T14:12:01Z

src/routes/safe/components/Apps/index.tsx

          const currentApp = list[index]

-          const appInfo: any = await getAppInfoFromUrl(currentApp.url)
+          const appInfo: SafeApp = await getAppInfoFromUrl(currentApp.url)


I think the type here will be inferred

also, removed the expected type for the `getAppInfoFromOrigin` calls as it is inferred

ghost · 2020-07-29T14:25:33Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

ghost · 2020-07-30T10:57:26Z

Travis automatic deployment:
https://pr1021--safereact.review.gnosisdev.com/app

Fix: debounce fetch apps

04776f4

nicosampler self-assigned this Jun 16, 2020

nicosampler requested review from Agupane, fernandomg and mmv08 June 16, 2020 16:45

nicosampler added the WIP label Jun 16, 2020

mmv08 suggested changes Jun 18, 2020

View reviewed changes

fernandomg added 2 commits July 22, 2020 11:55

Merge branch 'development' into issue-999

7415444

Merge branch 'development' into issue-999

cd1444e

fernandomg marked this pull request as draft July 22, 2020 18:34

fernandomg added Bug 🐛 Something isn't working Major Needs to be fixed for immediate next public release. and removed WIP labels Jul 22, 2020

fernandomg added 5 commits July 23, 2020 12:57

refactor: fix AddAppForm name and add missing types

da85abe

add use-lodash-debounce hook to test debounce functionality

bc6be07

I'm planning to remove this dependency, as it requires to also install `lodash.debounce`. I prefer to implement it ad-hoc.

refactor AddAppForm to use the observable pattern

2f30977

memoize getAppInfoFromUrl to prevent requesting the same informatio…

2046e87

…n over and over

Merge branch 'development' into issue-999

46cd3a7

fernandomg added 2 commits July 23, 2020 13:10

prevent requesting data if url is not valid

28e5b26

remove logging

6ba3f9d

refactor: rename isSubmitDisabled to onSubmitButtonStatusChange prop

bfef944

fernandomg added 5 commits July 24, 2020 16:57

refactor: rename agreement to agreementAccepted

d679bf9

also, moved `initialValues` to a constant `INITIAL_VALUES` outside the component

refactor: reimplement useDebounce hook in-app

7c6c9cd

refactor: extract app manifest verification to a helper function

c80db53

also fixed types

Merge branch 'development' into issue-999

f3edd9b

fix: prevent accessing contentWindow if iframe is null

41330c5

Merge branch 'fix/contentWindow-of-null' into issue-999

8df4101

fernandomg requested review from Agupane and mmv08 July 24, 2020 20:38

Merge branch 'development' into issue-999

fff6c44

Merge branch 'development' into issue-999

a2bf47a

francovenica approved these changes Jul 27, 2020

View reviewed changes

mmv08 approved these changes Jul 27, 2020

View reviewed changes

fernandomg added 2 commits July 29, 2020 10:39

Merge branch 'development' into issue-999

9c4d4ae

fix: getAppInfoFromOrigin return type

cafafc8

also, removed the expected type for the `getAppInfoFromOrigin` calls as it is inferred

Merge branch 'development' into issue-999

280c100

Agupane approved these changes Jul 30, 2020

View reviewed changes

mmv08 merged commit 251da31 into development Jul 30, 2020

mmv08 deleted the issue-999 branch July 30, 2020 14:28

mmv08 mentioned this pull request Aug 7, 2020

Release: v2.9.0 #1220

Merged

Conversation

nicosampler commented Jun 16, 2020

Uh oh!

ghost commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

mmv08 Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

nicosampler Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

Agupane Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

fernandomg Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

mmv08 commented Jul 7, 2020

Uh oh!

mmv08 commented Jul 16, 2020

Uh oh!

nicosampler commented Jul 16, 2020

Uh oh!

mmv08 commented Jul 16, 2020

Uh oh!

nicosampler commented Jul 16, 2020

Uh oh!

mmv08 commented Jul 16, 2020

Uh oh!

ghost commented Jul 22, 2020

Uh oh!

github-actions bot commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

[warning] @typescript-eslint/no-non-null-assertion

Uh oh!

ghost commented Jul 23, 2020

Uh oh!

ghost commented Jul 24, 2020

Uh oh!

francovenica commented Jul 24, 2020

Uh oh!

fernandomg commented Jul 24, 2020

Uh oh!

nicosampler commented Jul 24, 2020

Uh oh!

mmv08 commented Jul 24, 2020

Uh oh!

ghost commented Jul 24, 2020

Uh oh!

ghost commented Jul 24, 2020

Uh oh!

francovenica commented Jul 27, 2020

Uh oh!

mmv08 Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 29, 2020

Uh oh!

ghost commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Jul 23, 2020 •

edited

Loading