Refactor major components to streamline logic #1589

Gozala · 2020-08-15T04:21:27Z

Problem Statement

As I worked on #1568 I had hard time figuring out what was happening where, in fact I left a TODO there because I was unable to figure out what make function in the bundles/files/utils was trying to do:

https://github.com/ipfs-shipyard/ipfs-webui/blob/16e15a25ab999d4912ea05ebb81e9bad67457770/src/bundles/files/utils.js#L12-L78

Then as I was trying to find what listenes to FILES_WRITE_UPDATED actions, I was able to find nothing. Later I discovered that make function does some trickery where it dispatches FILES_X_STARTED, FILES_X_FINISHED actions and also does some extra stuff on finish. This all makes really difficult to know what happens where.

Solution

This pull request is attempt to streamline everything, so that looking at the specific doX function tells you all you need to know without having to look around other places to get a sense what happens after finish etc... In order to accomplish that it switches from FILES_X_STARTED / FILES_X_FINISHED style action dispatch to making FILES_X actions dispatch events with job field that that updates it's state (as in state machine) from Idle -> Active -> (Done | Failed).

This also gets rid of all the post success followups:

https://github.com/ipfs-shipyard/ipfs-webui/blob/16e15a25ab999d4912ea05ebb81e9bad67457770/src/bundles/files/utils.js#L42-L60

And post failure followups:

https://github.com/ipfs-shipyard/ipfs-webui/blob/16e15a25ab999d4912ea05ebb81e9bad67457770/src/bundles/files/utils.js#L61-L76

By using try { ... } finally { ... } in in the doX functions, so it's plain and clear:

https://github.com/ipfs-shipyard/ipfs-webui/blob/f235c331c80068a023359efd50cbef48a6008d9b/src/bundles/files/actions.js#L324-L338

Pull request also introduces idea of non atomic tasks that (start, make progress, finish or fail) so that doX tasks that report progress don't need to do custom workarounds:

https://github.com/ipfs-shipyard/ipfs-webui/blob/16e15a25ab999d4912ea05ebb81e9bad67457770/src/bundles/files/actions.js#L298-L310

Instead spawned tasks (that follow above described state machine) can yield, to produce update states:

https://github.com/ipfs-shipyard/ipfs-webui/blob/f235c331c80068a023359efd50cbef48a6008d9b/src/bundles/files/actions.js#L243-L251

And it adds typing for all of this so any inconsistencies could caught early on. In the process I also fixed bunch that existed.

lidel

I am not familiar with redux enough to provide meaningful feedback there, but switching from post-success and post-failure to a simple try/catch makes it easier to reason about and maintain 👍

Small ask: as noted during our chat today, we need to understand if/how this change impacts the way WebUI's opt-in Countly metrics are gathered (not a blocker, but we should know the impact before merging this).

@rafaelramalho19 mind blocking some time this week to review this?

(cc @olizilla / @hacdias in case they are around and have thoughts)

Gozala · 2020-08-18T22:01:48Z

Small ask: as noted during our chat today, we need to understand if/how this change impacts the way WebUI's opt-in Countly metrics are gathered (not a blocker, but we should know the impact before merging this).

This was a good call @lidel, indeed analytics seems to use mix of regexps and strings to hook into these actions, which is also a reason I have missed those:

https://github.com/ipfs-shipyard/ipfs-webui/blob/master/src/bundles/analytics.js#L6-L20

I will look into updating that module to take into account the changes made here.

lidel

@Gozala I am mildly in favor, but afraid I am not aware of all ramifications of this refactor.
Mind scheduling a call with @olizilla this or next week to go over changes and unblock this?
(Ideally, me and @rafaelramalho19 would join it too)

rafaelramalho19 · 2020-08-18T17:10:04Z

src/bundles/files/index.js

+      }
+    }
+    case 'Active': {
+      const { pending, rest } = pullPendig(state.pending, job)


You have a typo in the function name 😄 it should be pullPending

src/bundles/files/index.js

src/bundles/files/utils.js

src/bundles/ipfs-provider.js

rafaelramalho19 · 2020-08-18T17:19:14Z

src/bundles/ipfs-provider.js

@@ -150,14 +158,20 @@ const readSetting = (id) => {
    }

    try {
-      return JSON.parse(setting)
+      return JSON.parse(setting || '')


Why are you fallbacking to ''? If you do JSON.parse('') it actually gives you the "Unexpected end of JSON input" error.

Because setting could be a null (e.g. if try above failed), while JSON.parse has type of (input:string) -> JSON. Passing string|null would be rejected by a type checker. setting || '' on the other hand has string type.

I would say that error in this scenario is an expected behavior.

rafaelramalho19 · 2020-08-18T17:20:29Z

src/bundles/ipfs-provider.js

@@ -150,14 +158,20 @@ const readSetting = (id) => {
    }

    try {
-      return JSON.parse(setting)
+      return JSON.parse(setting || '')
    } catch (_) {
      // res was probably a string, so pass it on.
      return setting


Maybe the '' fallback should be here?

Well return type of this function is string | null so null is fine. It maybe reasonable to change type to string and check for '', but it would be bit trickier to catch all the places where '' case isn't considered because type checker won't really help, unlike spotting cases where string | null is treated as string.

rafaelramalho19 · 2020-08-18T17:23:01Z

src/bundles/ipfs-provider.js

+  /**
+   * @returns {function(Context):Promise<void>}
+   */
+  doInitIpfs: () => async (context) => {


Any reason to refactor from store to context? From the redux bundler documentation, they always call this variable store and not context.

Doing store.dispatch seems more intuitive than context.dispatch 🤔

Turns out what redux-bundle passes into doX style things isn't a store but rather it's own context thing that contains store (see https://reduxbundler.com/api-reference/bundle#bundle-doxA) I think I originally was calling it a store as well, but then it got confusing because in some cases it is store and in others it isn't.

tsconfig.json

rafaelramalho19 · 2020-09-03T15:37:20Z

src/bundles/ipfs-provider.js

+        }
+        case 'Exit': {
+          const { result } = task
+          if (result.ok) {


I would invert this into early exit

if (!result.ok) { return { ...state, ready: false, failed: true } } // ...

lidel · 2020-09-15T16:17:12Z

@Gozala mind rebasing this PR?
We really want to merge this before #1615 (if possible)

Co-authored-by: Rafael Ramalho <rafazelramalho19@gmail.com>

Gozala · 2020-09-16T01:08:47Z

@Gozala mind rebasing this PR?
We really want to merge this before #1615 (if possible)

Just pushed the version that merges changes from master. It end up been a bit more work than I have anticipated & I had not had a chance to do a testing with merged version yet.

lidel

LGTM, no regressions as far I can tell, and should make progress reporting easier to reason about.

@Gozala what would be the next step for having visual feedback during file import? Any PRs/issues in other repos that I could help landing/unblocking?

@rafaelramalho19 please block some time this week to do another pass at this 🙏

lidel · 2020-09-16T21:39:33Z

@types/redux-bundler/index.d.ts

+declare module "redux-bundler" {
+
+  interface CreateSelector {
+    <State, I, O>(n1: string, f: (inn: I) => O): (state: State) => O,
+    <State, I1, I2, O>(n1: string, n2: string, f: (i1: I1, i2: I2) => O): (state: State) => O
+    <State, I1, I2, I3, O>(n1: string, n2: string, n3: string, f: (i1: I1, i2: I2, i3: I3) => O): (state: State) => O
+    <State, I1, I2, I3, I4, O>(n1: string, n2: string, n3: string, n4: string, f: (i1: I1, i2: I2, i3: I3, i4: I4) => O): (state: State) => O
+    <State, I1, I2, I3, I4, I5, O>(n1: string, n2: string, n3: string, n4: string, n5: string, f: (i1: I1, i2: I2, i3: I3, i4: I4, i5: I5) => O): (state: State) => O
+
+    <State, I1, O>(s1: (state: State) => I1, f: (inn: I1) => O): (state: State) => O,
+    <State, I1, I2, O>(s1: (state: State) => I1, s2: (state: State) => I2, f: (i1: I1, i2: I2) => O): (state: State) => O
+    <State, I1, I2, I3, O>(s1: (state: State) => I1, s2: (state: State) => I2, s3: (state: State) => I3, f: (i1: I1, i2: I2, i3: I3) => O): (state: State) => O
+    <State, I1, I2, I3, I4, O>(s1: (state: State) => I1, s2: (state: State) => I2, s3: (state: State) => I3, s4: (state: State) => I4, f: (i1: I1, i2: I2, i3: I3, i4: I4) => O): (state: State) => O
+    <State, I1, I2, I3, I4, I5, O>(s1: (state: State) => I1, s2: (state: State) => I2, s3: (state: State) => I3, s4: (state: State) => I4, s5: (state: State) => I5, f: (i1: I1, i2: I2, i3: I3, i4: I4, i5: I5) => O): (state: State) => O
+  }


💭 OOF, I have bad feelings about maintaining this long term.

@Gozala thoughts on how feasible it is to pick HenrikJoreteg/redux-bundler#69 up and add those types to the upstream package (with GitHub action that tests them there)?

I don't believe it is worth it honestly. Because it is not really possible to add proper typings to the redux-bundler. What I do here is just provide some subset which also requires very particular use of redux-bundler (splitting actions and selectors, which seems to go against it's design goals), and even then have to manually annotate. Given these limitations, I doubt it would be accepted, I also don't think the effort to do that is going to pay off.

If we were to invest more time into this, I would rather consider using redux-bundler alternatives that would be more type friendly / ready than redux-bundler is.

rafaelramalho19

After the 2 small nits it's good to merge.

Good job 👍

src/bundles/files/index.js

src/bundles/analytics.js

Gozala added 4 commits June 30, 2020 17:54

feat: web native file add

d925c3b

chore: merge upstream

e6336ed

chore: integarte new ipfs & upload/download events

e636d2b

chore: code refactor & cleanup

f235c33

Gozala requested review from lidel and rafaelramalho19 August 15, 2020 04:21

fix: view that assumed older state structure

64702a5

lidel reviewed Aug 17, 2020

View reviewed changes

Gozala added 6 commits August 20, 2020 23:32

fix: refactor other bundles

6043e97

save draft

d4d8561

fix: type incompatibilities

9f6512a

chore: turn perform and spawn into action creators

f2ac8f1

chore: update docs

01c529d

fix: introduces regressions

0fa1188

Gozala requested a review from lidel August 22, 2020 01:01

rafaelramalho19 mentioned this pull request Aug 24, 2020

Files screen: error when renaming file #1537

Closed

lidel added this to the v2.11 milestone Aug 25, 2020

lidel reviewed Aug 26, 2020

View reviewed changes

jessicaschilling modified the milestones: v2.11, v2.12 Sep 1, 2020

rafaelramalho19 suggested changes Sep 3, 2020

View reviewed changes

rafaelramalho19 reviewed Sep 3, 2020

View reviewed changes

Gozala and others added 3 commits September 15, 2020 09:41

Update src/bundles/files/utils.js

aa1d733

Co-authored-by: Rafael Ramalho <rafazelramalho19@gmail.com>

Apply suggestions from code review

21a49b2

Co-authored-by: Rafael Ramalho <rafazelramalho19@gmail.com>

Merge remote-tracking branch 'upstream/master' into code-refactor

d9343c7

Gozala force-pushed the code-refactor branch from fcd81d3 to d9343c7 Compare September 16, 2020 01:05

chore: put back \n at end of json file

6e01b90

fix: regression introuded by missing - char.

448a28c

Gozala requested review from rafaelramalho19 and lidel September 16, 2020 06:27

lidel approved these changes Sep 16, 2020

View reviewed changes

lidel mentioned this pull request Sep 17, 2020

File(s) import progress #1594

Closed

rafaelramalho19 suggested changes Sep 21, 2020

View reviewed changes

src/bundles/files/index.js Outdated Show resolved Hide resolved

src/bundles/analytics.js Outdated Show resolved Hide resolved

rafaelramalho19 self-requested a review September 21, 2020 17:44

fix: address review comments

271b90e

Gozala merged commit 056d389 into master Sep 21, 2020

Gozala deleted the code-refactor branch September 21, 2020 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor major components to streamline logic #1589

Refactor major components to streamline logic #1589

Gozala commented Aug 15, 2020 •

edited

Loading

lidel left a comment

Gozala commented Aug 18, 2020

lidel left a comment •

edited

Loading

rafaelramalho19 Aug 18, 2020

rafaelramalho19 Aug 18, 2020

Gozala Sep 15, 2020

rafaelramalho19 Aug 18, 2020

Gozala Sep 15, 2020

rafaelramalho19 Aug 18, 2020

Gozala Sep 15, 2020

rafaelramalho19 Sep 3, 2020

Gozala Sep 15, 2020

lidel commented Sep 15, 2020

Gozala commented Sep 16, 2020

lidel left a comment •

edited

Loading

lidel Sep 16, 2020

Gozala Sep 17, 2020 •

edited

Loading

rafaelramalho19 left a comment

Refactor major components to streamline logic #1589

Refactor major components to streamline logic #1589

Conversation

Gozala commented Aug 15, 2020 • edited Loading

Problem Statement

Solution

lidel left a comment

Choose a reason for hiding this comment

Gozala commented Aug 18, 2020

lidel left a comment • edited Loading

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

lidel commented Sep 15, 2020

Gozala commented Sep 16, 2020

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

rafaelramalho19 left a comment

Choose a reason for hiding this comment

Gozala commented Aug 15, 2020 •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading

Gozala Sep 17, 2020 •

edited

Loading