Make state immutable in reducers/effects #46

toddself · 2016-06-15T04:35:09Z

Call Object.freeze on state when passing it into to the reducers or effects
so that state cannot be modified except when being returned from an emitter.

Add tests to prove state is immutable in these scenarios.

toddself · 2016-06-15T04:40:17Z

So! The current tests run in node which means you can't call send.

So I made a test file that you can browserify and run in the client.

However, setting up automated testing for browser tests is super opinionated so I didn't set up a way to actually, you know, run the tests.

We use karma for scripto for client testing (and xvfb for doing it headless on CI). However, karma is very heavy and requires a TON of modules and plugins. but it also is extremely reliable, and gets you coverage reports easily. But, since it's also XVFB for automated testing, I still can't get it set up correctly on travis-ci (but can do it on circle-ci).

I've also done testling, which is super minimal, but doesn't get you coverage easily and still requires XVFB.

Zuul is great, but you can't run it in an automated fashion without setting up sauce labs support (I have an extremely old branch where I tried to implement local test running but never finished).

So like, um, I have no clue how you want to commit to running these tests, but like this will open it and run it...

echo '<title>test</title><body><script src="bundle.js"></script>' > index.html
../node_modules/bin/browserify browser.js > bundle.js
open index.html

(Or xdg-open if you're on linux. I have no clue how to do this on windows lol)

yoshuawuyts · 2016-06-15T11:32:06Z

Ooh, love this patch - minor version I reckon. Just needs tests fixed and we can merge.

Browser tests are def tricky - perhaps just add these tests for now, but exclude them from CI? - that way Travis stays green and we can save ourselves some work in the future.

I've done a bit of work for tests / coverage here - #24. It uses smokestack which can integrate with saucelabs; I reckon using sauce might be the easiest?

queckezz · 2016-06-15T11:43:39Z

Another alternative to consider might be tape-run. Basically runs your code through a headless electron instance. No integration with saucelabs ofc though, so v8 only. I run it like this:

browserify --debug test/index.js | tape-run --render="tap-spec"

Edit: Sorry, should have written this into #24

Call Object.freeze on state when passing it into to the reducers or effects so that state cannot be modified except when being returned from an emitter. Add tests to prove state is immutable in these scenarios.

toddself · 2016-06-15T14:47:28Z

Oooooh I hadn't seen tape-run and it's instructions for using electron! That would greatly simplify some other only-chrome stuff in my life :).

Interestingly, you can't easily mark a test as "skip" when you run tape tests through the node executable. Adding {skip: true} at either level makes node simply exit after running browser test (since it comes first in the glob), and even wrapping it in an if statement causes node to exit after processing the file.

I renamed it so it comes later in the glob pattern (and since node will exit after the first file it will never execute), but this feels really hacky?

Better ideas or solutions?

Also added some documentation to explain that state is frozen within a reducer or an effect just to be explicit about it.

toddself · 2016-06-16T23:46:00Z

So I tried to use Zuul since then it can run in sauce labs for free using "open sauce" which means you can target a ton of browsers, but Zuul uses an older version of browserify-istanbul, which relies on an older version of esprima which is unable to parse template tag syntax.

I opened a PR to update to the latest version of browserify-istanbul, but that's a dead-end until then defunctzombie/zuul#289

I do have this running under karma in my project, but the list of requirements is pretty heavy (although some sub-deps of choo require karma already so we're installing it as it is).

Here's the minimal list I've found I need to just support tape and istanbul via browserify:

    "browserify": "^13.0.1",
    "browserify-istanbul": "^2.0.0",
    "istanbul": "^0.4.3",
    "karma": "^0.13.22",
    "karma-browserify": "^5.0.5",
    "karma-chrome-launcher": "^1.0.1",
    "karma-coverage": "^1.0.0",
    "karma-script-launcher": "^1.0.0",
    "karma-tap": "^1.0.4",
    "tape": "^4.5.1",

That feels kind of heavy, but it works. Except for in travis-ci.

yoshuawuyts · 2016-06-20T13:54:56Z

This patch is looking great by the way - testing, merging and publishing as minor ✨

yoshuawuyts · 2016-06-20T14:00:18Z

oops; so the http example is failing - digging in

yoshuawuyts · 2016-06-20T14:05:25Z

Ah yeah, so the problem here is that Object.freeze mutates an existing object, rather than create a new copy - the fact that a single action can trigger both an effect and reducer is problematic - I think the cleanest resolution is to only allow for a single call - that would be a breaking change.

I think we should clean this up and publish in v3. @toddself would you be willing to do that?

anandthakker · 2016-06-24T11:36:06Z

@yoshuawuyts @toddself Hey, just a heads up -- I've lost the reference to where I read about this, but I distinctly remember reading that Object.freeze has some pretty bad performance characteristics in some browsers (maybe Safari? not sure). Would you consider guarding this with a check for process.env.NODE_ENV === 'production', so that it could be easily stripped out of a production build?

anandthakker · 2016-06-24T11:39:55Z

the fact that a single action can trigger both an effect and reducer is problematic - I think the cleanest resolution is to only allow for a single call

@yoshuawuyts interesting; could you say a bit more about this? If I'm understanding you correctly, this would effectively remove "actions" as a concept independent of effects and reducers--is that right?

yoshuawuyts · 2016-06-24T11:59:18Z

Object.freeze has some pretty bad performance characteristics in some browsers (maybe Safari? not sure).

Oh yeah, that's a good call. We could def add an argument that can be passed on start - I've found it's generally not a good idea to have env var switches live within packages.

If I'm understanding you correctly, this would effectively remove "actions" as a concept independent of effects and reducers--is that right?

Ummm, maybe? The way I view actions is as a single trigger on any of the stores - a 1:1 mapping if you will; quite similar to a function call. Having it trigger multiple things at once seems... well confusing haha

Hope this answers your questions!

anandthakker · 2016-06-24T12:28:55Z

The way I view actions is as a single trigger on any of the stores - a 1:1 mapping if you will; quite similar to a function call. Having it trigger multiple things at once seems... well confusing haha

Interesting. In my work, I've been thinking of "actions" as a separate abstraction, basically representing any "verb" within the system (some of which map directly to user actions like selectSomeItem or cancelSomething, others of which are more internal like fetchSomeData or updateBlahStatus). Coming from that point of view, I feel that it can sometimes make sense for an action to trigger multiple things, because I think there isn't always a 1:1 mapping between "verbs" and stores. But if there's going to be a 1:1 mapping, then would it be simpler to eliminate actions altogether, in favor of an API like (state, effects, stores) => { ... stores['namespace:reducerName'](args) ... }?

toddself · 2016-06-24T13:31:52Z

I have it set up similarly but rather use effects to trigger multiple reducers to react to input.

The user might perform some action which causes several parts of the state to update, so i trigger an effect which in turn triggers all the single modification reducers to fire that are necessary.

On Jun 24, 2016, at 05:28, Anand Thakker notifications@github.com wrote:

The way I view actions is as a single trigger on any of the stores - a 1:1 mapping if you will; quite similar to a function call. Having it trigger multiple things at once seems... well confusing haha

Interesting. In my work, I've been thinking of "actions" as a separate abstraction, basically representing any "verb" within the system (some of which map directly to user actions like selectSomeItem or cancelSomething, others of which are more internal like fetchSomeData or updateBlahStatus). Coming from that point of view, I feel that it can sometimes make sense for an action to trigger multiple things, because I think there isn't always a 1:1 mapping between "verbs" and stores. But if there's going to be a 1:1 mapping, then would it be simpler to eliminate actions altogether, in favor of an API like (state, effects, stores) => { ... stores'namespace:reducerName' ... }?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

anandthakker · 2016-06-24T13:36:11Z

Yeah, I can see how that makes sense, although again, if there's going to be a 1-1 mapping between actions and effects||reducers, then it seems to me like the "actions" concept is unnecessary...

toddself · 2016-06-24T13:41:28Z

It's more of one-to-many. One effect can trigger several reducers, but
a reducer only ever changes one part of the state and will never call another
reducer.

On Fri, Jun 24, 2016 at 06:36:12AM -0700, Anand Thakker wrote:

Yeah, I can see how that makes sense, although again, if there's going to be a 1-1 mapping between actions and effects||reducers, then it seems to me like the "actions" concept is unnecessary...

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#46 (comment)

toddself · 2016-06-24T13:42:07Z

Here is an example:
https://github.com/toddself/boxcar/blob/master/effects/move.js

On Fri, Jun 24, 2016 at 06:36:12AM -0700, Anand Thakker wrote:

Yeah, I can see how that makes sense, although again, if there's going to be a 1-1 mapping between actions and effects||reducers, then it seems to me like the "actions" concept is unnecessary...

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#46 (comment)

anandthakker · 2016-06-24T13:43:02Z

It's more of one-to-many. One effect can trigger several reducers, but a reducer only ever changes one part of the state and will never call another reducer.

effects-reducers is 1-to-many, but I thought @yoshuawuyts was proposing above that actions-{reducers or effects} should be 1-1

yoshuawuyts · 2016-06-24T15:28:15Z

(...) actions-{reducers or effects} should be 1-1

Yup you're both right. One action can trigger either one effect or one reducer; effects can emit multiple actions. Not the current behavior, but think it would make more sense

anandthakker · 2016-06-24T15:34:46Z

Yup you're both right. One action can trigger either one effect or one reducer; effects can emit multiple actions. Not the current behavior, but think it would make more sense

@yoshuawuyts I can definitely see your point on the clarity/simplicity of doing this, but it still leaves me with the question about what value is then being added by the "action" concept? Since each action would simply map directly to either a reducer or effect, they really are equivalent to function calls (like you said above) -- so at that point, why is the indirection through send(...) necessary or preferable to an API where you just call methods directly?

yoshuawuyts · 2016-06-24T15:45:46Z

why is the indirection through send(...) necessary or preferable to an API where you just call methods directly

Because multiplexing. A function call requires knowledge of which function you're calling, and importing it explicitly from the location it's declared. By using a message bus, function calling becomes centralized and thus more loosely coupled. This enables better tracing, logging and upgrading parts of the application without touching other parts. It also clearly signals that it's a different method of calling functions; one where callbacks or returning eventual values doesn't fly - thus making for a better enforcement of the unidirectional data flow (despite there always being ways of cheating around it).

anandthakker · 2016-06-24T16:04:32Z

A function call requires knowledge of which function you're calling, and importing it explicitly from the location it's declared. By using a message bus, function calling becomes centralized and thus more loosely coupled. This enables better tracing, logging and upgrading parts of the application without touching other parts.

Ah, yeah: I totally agree on the value of a message bus internally. What I was thinking/wondering about was not plain function calls, but rather an API where instead of getting send as a parameter, views (and effects, etc.) would get something like { effects, stores }, and where stores['thing1'](data) or effects['thing2'](data) would be the exact equivalent of send('thing1', data) or send('thing2', data)...

It also clearly signals that it's a different method of calling functions; one where callbacks or returning eventual values doesn't fly

... but I hadn't thought of this point, which I can see being a downside of the thing I'm describing above. I guess now that I've written it out more explicitly, the thing I was suggesting adds more magic and more internal machinery for little gain.

Thanks for engaging with my speculations and questions!

toddself · 2016-06-24T16:22:23Z

@yoshuawuyts agreed on the fact that Object.keys(reducers).concat(Object.keys(effects)) should be a unique set otherwise it makes it hard to reason about order of operations when you trigger something that fires and effect and a reducer.

yoshuawuyts · 2016-06-26T20:52:00Z

@toddself Given that this is moved to https://github.com/yoshuawuyts/choo/tree/browser-tests and barracks I think it's safe to close right? ✨

toddself · 2016-06-26T21:06:00Z

💯

Make state immutable in reducers/effects

0db9961

Call Object.freeze on state when passing it into to the reducers or effects so that state cannot be modified except when being returned from an emitter. Add tests to prove state is immutable in these scenarios.

toddself force-pushed the object-freeze branch from ba2c505 to 0db9961 Compare June 15, 2016 14:45

yoshuawuyts added this to the 3.0.0 milestone Jun 20, 2016

yoshuawuyts added feature labels Jun 20, 2016

yoshuawuyts mentioned this pull request Jun 22, 2016

browser support? #69

Closed

yoshuawuyts closed this Jun 26, 2016

Make state immutable in reducers/effects #46

Make state immutable in reducers/effects #46

Uh oh!

Conversation

toddself commented Jun 15, 2016

Uh oh!

toddself commented Jun 15, 2016

Uh oh!

yoshuawuyts commented Jun 15, 2016

Uh oh!

queckezz commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddself commented Jun 15, 2016

Uh oh!

toddself commented Jun 16, 2016

Uh oh!

yoshuawuyts commented Jun 20, 2016

Uh oh!

yoshuawuyts commented Jun 20, 2016

Uh oh!

yoshuawuyts commented Jun 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

yoshuawuyts commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

toddself commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

toddself commented Jun 24, 2016

Uh oh!

toddself commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

yoshuawuyts commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

yoshuawuyts commented Jun 24, 2016

Uh oh!

anandthakker commented Jun 24, 2016

Uh oh!

toddself commented Jun 24, 2016

Uh oh!

yoshuawuyts commented Jun 26, 2016

Uh oh!

toddself commented Jun 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

queckezz commented Jun 15, 2016 •

edited

Loading

yoshuawuyts commented Jun 20, 2016 •

edited

Loading