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

[minor] Th͏e Da҉rk Pońy Lo͘r͠d HE ́C͡OM̴E̸S #26

Closed
wants to merge 1 commit into from

Conversation

ahdinosaur
Copy link
Owner

@ahdinosaur ahdinosaur commented Jul 21, 2016

delay actions until next tick to keep Zalgo away

use case is a route handler like

['/', (params, model, dispatch) => {
  if (model.account) {
    dispatch(navigate('dashboard'))
  }

  return landingView(model, dispatch)
]

which causes an infinite loop. if i add a variable that ensures it only runs once, it works. with this change, it also works.

probably should test to make sure this is an actual bug, not just the heebie jeebies.

@codecov-io
Copy link

codecov-io commented Jul 21, 2016

Current coverage is 100% (diff: 100%)

Merging #26 into master will not change coverage

@@           master   #26   diff @@
===================================
  Files           5     5          
  Lines          76    80     +4   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           76    80     +4   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update e9552f2...6180fb1

@data-doge
Copy link

data-doge commented Jul 21, 2016

lol great PR title

@ahdinosaur
Copy link
Owner Author

ahdinosaur commented Jul 22, 2016

hehe thanks @data-doge, somewhat stolen from "Designing APIs for Asynchrony". 😄

so i think we actually want to avoid merging this pull request if possible, since delaying every action until after next tick is bad for snappy user interfaces.

but i'm having trouble reproducing this, everything i try seems to work fine, so there might just be something very weird with the setup i had that caused Zalgo to͞ ̀c͡ǫ̡͢m̵e̢̡͢ ̛̀̕ơu҉̕t ͜a̵ņd̶̢ ͠p̵͘l̸̷ay͏̸́. i'll continue to investigate.

@ahdinosaur
Copy link
Owner Author

ahdinosaur commented Jul 22, 2016

oh ho hum, here we go.

in my example, dispatch(navigate('dashboard')) is really dispatch(run(go('dashboard')), where go is an effect creator and run is an action creator that corresponds to an update handler like (model, effect) => { model, effect }. this means we want to dispatch an effect, but in doing so also updates the model.

in this case the model should be the same, but due to some overzealous xtend for each namespace in my app i'm creating a new object every time which thwarts the "only update if new model" feature.

so i wrote a test case that i hoped would pass (meaning there was an infinite loop):

const test = require('tape')
const defer = require('pull-defer')
const extend = require('extend')
const inu = require('../')
const pull = inu.pull

test('infinite loop if model always new and effect needs to run', function (t) {
  const app = {
    init: function () {
      return { model: {}, effect: 'INIT' }
    },
    update: function (model, action) {
      console.log('update', action)
      switch (action) {
        case 'TICK':
          return { model: {}, effect: 'NEXT' }
        case 'DONE':
          return { model: false }
      }
      return { model: model }
    },
    view: function (model, dispatch) {
      console.log('view', model)
      if (model) return dispatch('TICK')
      t.ok(false)
    },
    run: function (effect) {
      switch (effect) {
        case 'NEXT':
          return pull.values(['DONE'])
      }
    }
  }
  const sources = inu.start(app)
})

but it doesn't cause an infinite loop. the only case i could get to infinite loop was if the effect was asynchronous and returned the necessary new action after the next tick.

then i had another idea: what if we dispatch the navigate action after next tick. this gives us a full tick in between each step of the possible infinite loop to sort our shit out.

['/', (params, model, dispatch) => {
  if (model.account) {
    process.nextTick(() => {
      dispatch(navigate('dashboard'))
    })
  }

  return landingView(model, dispatch)
]

and it works. 🎉

maybe should take some of these learnings and add them to a Frequently Asked Questions section.

ahdinosaur added a commit that referenced this pull request Jul 22, 2016
ahdinosaur added a commit that referenced this pull request Jul 22, 2016
@ahdinosaur ahdinosaur mentioned this pull request Jul 22, 2016
10 tasks
@ahdinosaur
Copy link
Owner Author

closing, made new issue for Frequently Asked Questions entry

@ahdinosaur ahdinosaur closed this Jul 22, 2016
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.

3 participants