-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Breaking refactor and re-implementation of effects. #6
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ound. I kind of like it.
mrozbarry
force-pushed
the
experiment/app-runner
branch
from
September 21, 2018 11:46
ead6ea5
to
16ddb41
Compare
mrozbarry
force-pushed
the
experiment/app-runner
branch
2 times, most recently
from
September 21, 2018 13:48
2258c4f
to
5ae1435
Compare
mrozbarry
force-pushed
the
experiment/app-runner
branch
from
September 21, 2018 13:56
5ae1435
to
515bddd
Compare
…mizations for app.js
…er documentation.
…onstrate a concrete idea.
…her than a message/update.
…k to the core effects.
mrozbarry
changed the title
WIP Experimental app running mechanism
Breaking refactor and re-implementation of effects.
Sep 26, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This branch represents an internal re-write of the piece that runs the app. Since the mainline now has decent test coverage of how the app is run, I am confident to make internal changes, along with some external.
Changes
Effects as messages
A big change that will affect the code other developers write is how effects are handled. Previously, effects looked like this:
I had two main thoughts: first, I don't really like using classes in functional code, and second, this looks much more complicated than it has to. After some thought, and a few renames, I came to this conclusion:
none
,defer
, andbatch
are the core effect types, which can be wrapped and extended to do more interesting behaviours like delays, fetching data, etc. If your effect does not match a core type, it is treated as an immediate message.Inspecting an effect, it may look something like:
Previously, effects were messages wrapped in promises, but outside of a deferred message, this was a shoe-horn, and not really the right data type for effects. I'm also using symbols for types so they don't collide with developer-provided message types.
init
isn't a functionOriginally, I liked init being a function since
update
, andsubscribe
where also functions, so it made the app code look more uniform, but it's become apparent to me that this is not at all necessary.Previously, giving an initial state and effect looked like this:
And now it looks like this:
No more middleware
This was a harder decision that I'm still really thinking through. Originally, I wanted to rename and restructure middleware to listeners, methods that had no return value or control over update, but would be called after each update with the message that triggered the update, and the next state.
Originally, I like the rename, but it became apparent to me that this was starting to build an unnecessary feature. I think the answer for middleware is encouraging developers to use high-order functions to wrap their main update method. I've demonstrated this in examples/cli/updateLogger.js.
Result as data
Once I removed
class Effect
, it was clear that the result type would need some thought put into it to align with the rest of ferp. I especially didn't like how exposing the Result type meant writing code to enforce the result states.Since middleware and the logger were removed, serialize wasn't necessary to keep, and I didn't like having an exposed constructor, so moving to plain objects seemed like the next best move, especially considering the success I had with the effects messages.
That said, the outward api is almost identical. Here is the original class based results:
And with the new result module:
One thing is I wanted to align better with Elm's maybe union, which is something like
type Maybe = Nothing | Just a
.I'm still on the fence with this even being in this library, since internals don't use it, and I only have a single example. I'd love to get feedback on whether or not this is something that ferp should provide.
Note: This is a bit of a living document while I continue to review and refactor/clean up code.