Middlewares #13

AmaranthineCodices · 2018-03-05T02:49:03Z

This fixes #5. I wanted to open this to allow code reviews.

TODO:

Unit tests
Built-in logging middleware, as a combination example/debugging tool
Should middlewares affect the store initialization action? (ref. Store initialization action should have a type field #12)
Make middlewares optional

Copy-pasted from my issue comment:

The API now:

- function Store.new(reducer, initialState)
+ function Store.new(reducer, initialState, middlewares)

Middlewares are applied left-to-right order - the rightmost middleware is invoked first.

Middlewares look like this:

local function loggerMiddleware(next)
    return function(store, action)
        print("action dispatched:", action.type)
        next(action)
        print("new state:")
        printTable(store:getState())
    end
end

Using this looks like:

local store = Store.new(reducer, initialState, { loggerMiddleware })
store:dispatch({
    type = "someAction",
    ...
})
-- prints "action dispatched: someAction"
-- then the new state

LPGhatguy · 2018-03-05T03:00:01Z

Looks like Travis caught this, but we should make sure that middlewares is an optional argument!

coveralls · 2018-03-05T17:55:38Z

Coverage increased (+0.2%) to 99.296% when pulling b3724d7 on AmaranthineCodices:middlewares into 38ef143 on Roblox:master.

LPGhatguy

Nice! Small feedback

LPGhatguy · 2018-03-05T18:55:52Z

lib/loggerMiddleware.lua

+local function loggerPlugin(next)
+	return function(store, action)
+		print("Action dispatched:", prettyPrint(action))
+		next(action)


Should middleware be passing store as an argument as well as the next action? I figured that the self argument for the main reducer would get clobbered here.

Oops! You're right. Fixed now!

LPGhatguy · 2018-03-05T19:00:54Z

lib/init.lua


 return {
 	Store = Store,
 	createReducer = createReducer,
 	combineReducers = combineReducers,
+	logger = loggerMiddleware,


Should we name logger loggerMiddleware in the public API?

I figured just calling it logger was okay, but I think loggerMiddleware is a better name in hindsight - it makes it clear that it's a middleware function, not something else.

LPGhatguy · 2018-03-05T19:07:35Z

lib/Store.spec.lua

+				end
+			})
+
+			expect(store.dispatch).to.never.equal(Store.dispatch)


I don't think we need to assert that this is the case -- if the middleware implementation didn't actually override the dispatch method, that would satisfy what we want still!

Good point!

LPGhatguy · 2018-03-05T19:08:13Z

lib/Store.spec.lua

+						next(store, action)
+					end
+				end
+			})


I think we should split out the middleware function (and maybe the reducer too?) to make this instantiation a little easier to read.

Fixed, should be good now.

LPGhatguy · 2018-03-05T19:51:54Z

Ooh, I just noticed that this tanks our coverage.

Is there a way that we can change the loggerMiddleware to accept an arbitrary replacement print function without harming the outward API too much? I don't think we want to spam our tests with random stdout, but a no-op or instrumented print would be great.

AmaranthineCodices · 2018-03-05T22:41:33Z

Done. The whole loggerMiddleware.outputFunction thing should never be documented; it's literally only there for testing purposes.

LPGhatguy · 2018-03-05T23:19:22Z

lib/loggerMiddleware.lua

+			loggerPlugin.outputFunction("State changed to:"..prettyPrint(store:getState()))
+		end
+	end
+})


Returning a not-actually-function thing from an API like this makes me kind of uncomfortable, but I'm not sure how else to structure this.

Neither am I, without adding another layer to the logger >.<

Having this be a table is still a blocker in my mind. I'd really like to validate the return values of middleware as they're applied to the store, otherwise it might be kind of tricky to debug them.

The only other API I can think of that would let you choose the output function would be:

local function loggerMiddleware(logFunction) logFunction = logFunction or print return function(_, next) return function(store, action) -- ... end end end

And then the primary use of the middleware would be

Rodux.Store.new(reducer, nil, { Rodux.loggerMiddleware() })

I think that's mostly fine?

AmaranthineCodices · 2018-03-05T23:28:51Z

Barring a sudden insight in how to simplify loggerMiddleware, I think this is ready to merge.

AmaranthineCodices · 2018-03-08T02:01:20Z

Any news on this @LPGhatguy? I'd like to get this landed and move on to #14 and #8 to pave the way for v1.0 to be released.

LPGhatguy · 2018-03-08T02:11:48Z

lib/loggerMiddleware.lua

+			loggerPlugin.outputFunction("State changed to:"..prettyPrint(store:getState()))
+		end
+	end
+})


Having this be a table is still a blocker in my mind. I'd really like to validate the return values of middleware as they're applied to the store, otherwise it might be kind of tricky to debug them.

The only other API I can think of that would let you choose the output function would be:

local function loggerMiddleware(logFunction) logFunction = logFunction or print return function(_, next) return function(store, action) -- ... end end end

And then the primary use of the middleware would be

Rodux.Store.new(reducer, nil, { Rodux.loggerMiddleware() })

I think that's mostly fine?

AmaranthineCodices · 2018-03-08T02:18:16Z

That's a lot better than the hacky table approach; I've switched things over.

LPGhatguy

Looks architecturally great! Last pieces are minor or just style nits.

LPGhatguy · 2018-03-08T02:23:42Z

lib/Store.spec.lua

+				type = "test"
+			})
+
+            expect(middlewareInvokeCount).to.equal(1)


It looks like this line got indented with spaces somehow!

LPGhatguy · 2018-03-08T02:24:36Z

lib/loggerMiddleware.lua

+		return function(store, action)
+			outputFunction("Action dispatched: "..prettyPrint(action))
+			next(store, action)
+			outputFunction("State changed to:"..prettyPrint(store:getState()))


Can we space out .. like we would other operators?

LPGhatguy · 2018-03-08T02:25:06Z

lib/loggerMiddleware.spec.lua

+			type = "test"
+		})
+
+		expect(outputCount >= 1).to.be.ok()


I think we can check that the outputCount is exactly one now that we've defined that middleware aren't run on the initial action.

That test will fail, actually - loggerMiddleware calls outputFunction twice. I wrote this test so that it isn't super tied to the exact number of calls in the logger. I'm not sure if that's the best approach, but it was the one that seemed like the best option.

Oh, huh, I forgot about that.

Should we just make loggerMiddleware only get invoked once at the end of the middleware, with both the new state and action? We probably don't want to produce two separate log messages.

LPGhatguy · 2018-03-08T02:25:28Z

lib/loggerMiddleware.lua

+		table.insert(outputBuffer, strKey)
+		table.insert(outputBuffer, " = ")
+
+		if type(value) == "table" then


We should use typeof here, since we normalized everywhere else!

LPGhatguy · 2018-03-08T02:26:17Z

lib/loggerMiddleware.lua

+		"{\n"
+	}
+
+	for key, value in pairs(t) do


We don't have to go through with this for the first version of the logger, but sorting the table keys so that we get stable output would be a good enhancement in the future.

In an ideal case, I'd actually like to switch the logger to displaying state deltas, so you only see what changed. That's something to revisit later, though.

LPGhatguy

Hooray! Middleware!

prototype middleware implementation

c4a6f4c

Make middlewares optional

e48e337

AmaranthineCodices mentioned this pull request Mar 5, 2018

Refactor thunks out of Store #14

Closed

AmaranthineCodices added 4 commits March 5, 2018 13:02

unit test middlewares

a9cb4df

fix and expand middleware test case

cd65be6

destruct store at the end of test

fd4b9ff

add prototype logger middleware

2a70536

LPGhatguy suggested changes Mar 5, 2018

View reviewed changes

AmaranthineCodices and others added 7 commits March 5, 2018 17:18

pass store to next

8f97443

logger -> loggerMiddleware

76c3cd5

Merge branch 'master' into middlewares

5e79fad

remove unnecessary expectation

025471a

break up middleware test for readability

af37c57

allow output function to be changed for test purposes

e1f2c81

unit-test loggerMiddleware

c49485a

LPGhatguy reviewed Mar 5, 2018

View reviewed changes

AmaranthineCodices changed the title ~~WiP: Middlewares~~ Middlewares Mar 5, 2018

LPGhatguy suggested changes Mar 8, 2018

View reviewed changes

Switch away from table for specifying logger output function

09f6736

LPGhatguy suggested changes Mar 8, 2018

View reviewed changes

AmaranthineCodices added 3 commits March 7, 2018 21:31

fix indentation (?!)

e9a90ec

clean up styling

b870da8

add a missing space

4773705

AmaranthineCodices added 3 commits March 7, 2018 21:38

fix test case

ede7248

this is what I get for not running tests locally

585699a

call outputFunction once

b3724d7

LPGhatguy approved these changes Mar 8, 2018

View reviewed changes

LPGhatguy merged commit 400422a into Roblox:master Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Middlewares #13

Middlewares #13

AmaranthineCodices commented Mar 5, 2018 •

edited

Loading

LPGhatguy commented Mar 5, 2018

coveralls commented Mar 5, 2018 •

edited

Loading

LPGhatguy left a comment

LPGhatguy Mar 5, 2018

AmaranthineCodices Mar 5, 2018

LPGhatguy Mar 5, 2018

AmaranthineCodices Mar 5, 2018

LPGhatguy Mar 5, 2018

AmaranthineCodices Mar 5, 2018

LPGhatguy Mar 5, 2018

AmaranthineCodices Mar 5, 2018

LPGhatguy commented Mar 5, 2018

AmaranthineCodices commented Mar 5, 2018

LPGhatguy Mar 5, 2018

AmaranthineCodices Mar 5, 2018

LPGhatguy Mar 8, 2018

AmaranthineCodices commented Mar 5, 2018

AmaranthineCodices commented Mar 8, 2018

LPGhatguy Mar 8, 2018

AmaranthineCodices commented Mar 8, 2018

LPGhatguy left a comment

LPGhatguy Mar 8, 2018

LPGhatguy Mar 8, 2018

LPGhatguy Mar 8, 2018

AmaranthineCodices Mar 8, 2018

LPGhatguy Mar 8, 2018

AmaranthineCodices Mar 8, 2018

LPGhatguy Mar 8, 2018

LPGhatguy Mar 8, 2018

AmaranthineCodices Mar 8, 2018

LPGhatguy left a comment

Middlewares #13

Middlewares #13

Conversation

AmaranthineCodices commented Mar 5, 2018 • edited Loading

LPGhatguy commented Mar 5, 2018

coveralls commented Mar 5, 2018 • edited Loading

LPGhatguy left a 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

LPGhatguy commented Mar 5, 2018

AmaranthineCodices commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmaranthineCodices commented Mar 5, 2018

AmaranthineCodices commented Mar 8, 2018

Choose a reason for hiding this comment

AmaranthineCodices commented Mar 8, 2018

LPGhatguy left a 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

Choose a reason for hiding this comment

LPGhatguy left a comment

Choose a reason for hiding this comment

AmaranthineCodices commented Mar 5, 2018 •

edited

Loading

coveralls commented Mar 5, 2018 •

edited

Loading