loading plugin counts the number of outstanding effects per effect, module, and global #245

crazy4groovy · 2018-03-06T04:19:04Z

I was having problems with the loading module resolving a pending effect too early, because out of a batch of dispatched effects, the first completed one set the loading state to false.

This implements an integer counter instead of a boolean value.

Also cleaned up the plugin tests slightly.

crazy4groovy · 2018-03-06T04:20:23Z

package.json

@@ -39,7 +39,8 @@
 "test": "jest"
 },
 "dependencies": {
- "redux": "^3.7.2"
+ "redux": "^3.7.2",
+ "redux-persist": "^5.9.1"


This was a strange thing, but some tests were failing without it...... dunno how it was working before?

I noticed that in your PR. Tests "should" pass with 'redux-persist' as a dev dependency.

crazy4groovy · 2018-03-06T04:21:36Z

plugins/loading/test/loading.test.js

+ },
+ effects: {
+ async timeout() {
+ await delay(200)


a more standard async delay call

crazy4groovy · 2018-03-06T04:22:54Z

plugins/loading/test/loading.test.js

 dispatch.count.timeout()
- expect(store.getState().loading.global).toBe(true)
+ expect(store.getState().loading.global).toBe(2)


this is the gist of the new functionality

crazy4groovy · 2018-03-06T04:23:16Z

plugins/loading/test/loading.test.js

+
+ dispatch.count.timeout()
+ dispatch.count.timeout()
+ expect(store.getState().loading.effects.count.timeout).toBe(2)


this is the gist of the new functionality

crazy4groovy · 2018-03-06T04:23:57Z

plugins/loading/test/loading.test.js

+ dispatch.count.timeout2()
+ expect(store.getState().loading.effects.count.timeout1).toBe(1)
+ expect(store.getState().loading.effects.count.timeout2).toBe(1)
+ expect(store.getState().loading.global).toBe(2)


making sure global increments for every dispatched effect

crazy4groovy · 2018-03-06T04:25:21Z

plugins/loading/src/index.ts

+ await origEffect(...props)
+ // waits for dispatch function to finish before calling "hide"
+ } finally {
+ dispatch.loading.hide({ name, action })


omit catch, but decrements counter even on an error

ShMcK · 2018-03-06T04:25:28Z

Note that the loading plugin will likely require some changes once #236 is merged. Working on this at the moment.

At that point, loading can be handled with 2 actions, rather than the current 3.

show loading & action
hide loading

ShMcK · 2018-03-06T05:28:57Z

@crazy4groovy - interesting idea - an integer count is certainly more useful than a boolean. I would use this, for its debugging value alone.

The only issue I can see is that users with typechecking (TypeScript, Flow) may find it annoying.

Do you think it's possible to make it an "opt in" feature?

crazy4groovy · 2018-03-06T14:13:03Z

@ShMcK interesting point about typechecking. Honestly I'm not strong on the topic so I don't have any insight, sorry. But I didn't see any boolean typing info that I was converting into number so maybe you can help me understand your concern with an example?

Boolean -> number is a breaking change, so opt-in would help maintain backwards-compatibility. That is often a smart move, unless the added feature completely trumps the existing functionality. Can you see any case where the new functionality is not better than current? Alternatively a v1.x (semver) bump may be the right move.

Thoughts?

ShMcK · 2018-03-06T15:28:12Z

Can you see any case where the new functionality is not better than current?

Nope. You have a good idea.

Help me understand your concern with an example

In JS, there is no issue with dynamically typing variables.

var loading = 0
if (loading) {
  // do something
}

But when using TypeScript, for example, the compiler expects variables to be statically typed.

var loading = 0
if (loading) {          // warning, expected a boolean
  // do something
}

Then you have to do some nasty coercion to make the compiler happy.

var loading = 0
if (!!loading) {
  // do something
}

I believe these two solutions can co-exist very simply.

store loading as a number
if users "opt in" to a config for "valueType" = "boolean", then the number is coerced into a boolean, but defaults to "number"

What do you think?

crazy4groovy · 2018-03-06T17:04:29Z

Then you have to do some nasty coercion to make the compiler happy.

I see. Yes, in JS world (or TS?) I would just expect to check loading like this:

var loading = 0
if (loading > 0) { // always boolean
  // do something
}

I'm not opposed to a config param here, but it's not obvious to me how to store state as a number, and retrieve it as a boolean. Except maybe a selector of some kind? A hint here would be interesting for me.

…odule, and global

crazy4groovy · 2018-03-08T19:45:59Z

^^ poke?

ShMcK · 2018-03-08T19:55:40Z

Sorry, I'll need a bit more time. I'll try to get back to you later this evening.

ShMcK · 2018-03-09T05:00:07Z

I think both solutions can co-exist. I came up with something like this:

const createLoadingAction = (valueFn) =>
  (show) =>
    (state, { name, action }: any) => ({
      ...state,
      global: valueFn[show](state.global),
      models: {
        ...state.models,
        [name]: valueFn[show](state.models[name]),
      },
      effects: {
        ...state.effects,
        [name]: {
          ...state.effects[name],
          [action]: valueFn[show](state.effects[name][action]),
        },
      },
    })

 let valueFn
 let defaultValue
  if (config.valueType === 'number') {
    valueFn = {
      hide: () => 0,
      show: (v) => v + 1,
    }
   defaultValue = 0
  } else {
    valueFn = {
      hide: () => false,
      show: () => true,
    }
    defaultValue = false
  }

  const loadingAction = createLoadingAction(valueFn)

  const loading: Model = {
    name: loadingModelName,
    reducers: {
      hide: loadingAction('hide'),
      show: loadingAction('show'),
    },
    state: {
      effects: {},
      global: defaultValue,
      models: {},
    },
  }

  /* add defaultValue where needed */

In this case, users can opt in to using numbers over booleans by adding a config.valueType = 'number'.

This way, TypeScript/Flow folks are happy, no breaking changes, and the README can emphasize that numbers are an excellent option. What do you think?

crazy4groovy · 2018-03-09T19:03:23Z

Interesting idea! Feels pretty complicated but it's an interesting concept, I'll try to take a look this weekend.

Also, I think the numerical hide isn't quire right, I think it should just tick down the number instead of setting it to 0.

ShMcK · 2018-03-09T19:10:43Z

Great, also have a look at #266. Can you think of a way to resolve this with the loading plugin?

crazy4groovy · 2018-03-12T04:15:59Z

plugins/loading/src/index.ts


 const loadingModelName = config.name || 'loading'
+
+ const converter =


I'm pretty happy with this solution, it keeps a "background" counter called cntState of dispatched effects, but converts it onto its public state as either a number or boolean.

crazy4groovy · 2018-03-12T04:18:37Z

plugins/loading/test/loading-asNumber.test.js

@@ -0,0 +1,327 @@
+const delay = ms => new Promise(r => setTimeout(r, ms))


There are a lot more tests now.. I just duplicated them and tested for boolean in one file, and number in this one.

crazy4groovy · 2018-03-12T04:19:02Z

plugins/loading/README.md

@@ -62,21 +62,28 @@ init({

 ## Options

-### name
+### asNumber


yay for docs! 👍

crazy4groovy · 2018-03-12T04:20:17Z

plugins/loading/examples/react-loading-example/src/index.js

@@ -6,7 +6,7 @@ import createLoadingPlugin from '@rematch/loading'
 import App from './App'
 import example from './example'

-const loadingPlugin = createLoadingPlugin()
+const loadingPlugin = createLoadingPlugin({ asNumber: true })


The example uses numbers, I think it's way more practical personally

… number

ShMcK · 2018-03-13T04:53:45Z

Quite an elegant solution with the "converter". Thanks for putting in all the effort @crazy4groovy, great work, and sorry this took so long. I really appreciate your patience.

ShMcK · 2018-03-13T05:22:41Z

Getting a strange error from the Rollup bundler.

TypeError: parse is not a function in lib/index.js

There is no "parse" in that file.

WIll have to look at this tomorrow morning before I'm able to publish as @rematch/[email protected].

crazy4groovy · 2018-03-13T12:41:43Z

Drat sorry. Here's a 100% shot in the dark to try: could be the export default statement I consolidated, try pulling it back out to the bottom maybe (I've just heard someone else mention that on a different project.)

crazy4groovy · 2018-03-13T14:45:24Z

Maybe this issue: rollup/rollup-plugin-commonjs#299

ShMcK · 2018-03-13T15:14:32Z

Something changed in Rollup. Before I could use the peer dependencies in the root "rematch" directory, but now I had to install them all as dev dependencies in the plugins.

ShMcK · 2018-03-13T15:24:22Z

Thanks for your help. Published as @rematch/[email protected].

semoal · 2020-09-11T15:08:56Z

@tianzhich

crazy4groovy force-pushed the master branch from b5d14fd to 80804b5 Compare March 6, 2018 04:19

crazy4groovy commented Mar 6, 2018

View reviewed changes

crazy4groovy force-pushed the master branch from 80804b5 to ab02d5c Compare March 6, 2018 04:31

loading plugin counts the number of outstanding effects per effect, m…

ff72db1

…odule, and global

crazy4groovy force-pushed the master branch from ab02d5c to ff72db1 Compare March 7, 2018 01:12

Merge branch 'master' into master

a54795d

ShMcK mentioned this pull request Mar 9, 2018

loading example not reset on second submit #266

Closed

loading plugin tweaks and simplifications

619d7ec

crazy4groovy force-pushed the master branch from c88e28b to 619d7ec Compare March 12, 2018 03:02

crazy4groovy commented Mar 12, 2018

View reviewed changes

plugins/loading/README.md

@@ -62,21 +62,28 @@ init({

## Options

### name

### asNumber

Copy link

Contributor Author

crazy4groovy Mar 12, 2018

Choose a reason for hiding this comment

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

yay for docs! 👍

crazy4groovy commented Mar 12, 2018

View reviewed changes

crazy4groovy force-pushed the master branch from ef0f7ee to d1b1a09 Compare March 12, 2018 04:22

lodaing plugin implements asNumber option, state is either boolean or…

77173f3

… number

crazy4groovy force-pushed the master branch from d1b1a09 to 77173f3 Compare March 12, 2018 04:35

ShMcK merged commit 5911a9b into rematch:master Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loading plugin counts the number of outstanding effects per effect, module, and global #245

loading plugin counts the number of outstanding effects per effect, module, and global #245

crazy4groovy commented Mar 6, 2018

crazy4groovy Mar 6, 2018

ShMcK Mar 6, 2018

crazy4groovy Mar 6, 2018

crazy4groovy Mar 6, 2018

crazy4groovy Mar 6, 2018

crazy4groovy Mar 6, 2018

crazy4groovy Mar 6, 2018

ShMcK commented Mar 6, 2018

ShMcK commented Mar 6, 2018

crazy4groovy commented Mar 6, 2018 •

edited

Loading

ShMcK commented Mar 6, 2018

crazy4groovy commented Mar 6, 2018 •

edited

Loading

crazy4groovy commented Mar 8, 2018

ShMcK commented Mar 8, 2018

ShMcK commented Mar 9, 2018 •

edited

Loading

crazy4groovy commented Mar 9, 2018

ShMcK commented Mar 9, 2018

crazy4groovy Mar 12, 2018

crazy4groovy Mar 12, 2018

crazy4groovy Mar 12, 2018

crazy4groovy Mar 12, 2018

ShMcK commented Mar 13, 2018

ShMcK commented Mar 13, 2018 •

edited

Loading

crazy4groovy commented Mar 13, 2018

crazy4groovy commented Mar 13, 2018

ShMcK commented Mar 13, 2018

ShMcK commented Mar 13, 2018

semoal commented Sep 11, 2020


		const loadingModelName = config.name \|\| 'loading'

		const converter =

		@@ -0,0 +1,327 @@
		const delay = ms => new Promise(r => setTimeout(r, ms))

loading plugin counts the number of outstanding effects per effect, module, and global #245

loading plugin counts the number of outstanding effects per effect, module, and global #245

Conversation

crazy4groovy commented Mar 6, 2018

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

ShMcK commented Mar 6, 2018

ShMcK commented Mar 6, 2018

crazy4groovy commented Mar 6, 2018 • edited Loading

ShMcK commented Mar 6, 2018

crazy4groovy commented Mar 6, 2018 • edited Loading

crazy4groovy commented Mar 8, 2018

ShMcK commented Mar 8, 2018

ShMcK commented Mar 9, 2018 • edited Loading

crazy4groovy commented Mar 9, 2018

ShMcK commented Mar 9, 2018

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

ShMcK commented Mar 13, 2018

ShMcK commented Mar 13, 2018 • edited Loading

crazy4groovy commented Mar 13, 2018

crazy4groovy commented Mar 13, 2018

ShMcK commented Mar 13, 2018

ShMcK commented Mar 13, 2018

semoal commented Sep 11, 2020

crazy4groovy commented Mar 6, 2018 •

edited

Loading

crazy4groovy commented Mar 6, 2018 •

edited

Loading

ShMcK commented Mar 9, 2018 •

edited

Loading

ShMcK commented Mar 13, 2018 •

edited

Loading