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

Return the state fragment to the caller if the action is a reducer. #243

Merged
merged 4 commits into from
Jun 23, 2017

Conversation

jorgebucaran
Copy link
Owner

@jorgebucaran jorgebucaran commented Jun 23, 2017

Now actions.myAction will return whatever the original myAction function returns. Before, this was only true if myAction returned a Promise.

Example

app({
...
  actions: {
    getState: state => state
  }
...
})

Diff: 0 bytes.

Fix #242.

Jorge Bucaran added 3 commits June 19, 2017 19:31
Now actions.myAction will return whatever the original myAction function
returns. Before, this was only true if myAction returned a Promise.

Fix #242.
@jorgebucaran jorgebucaran requested a review from zaceno June 23, 2017 02:04
@codecov
Copy link

codecov bot commented Jun 23, 2017

Codecov Report

Merging #243 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #243   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         165    165           
  Branches       45     45           
=====================================
  Hits          165    165
Impacted Files Coverage Δ
src/app.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 316b2a8...86f3a08. Read the comment docs.

@zaceno zaceno merged commit 9173351 into master Jun 23, 2017
@jorgebucaran
Copy link
Owner Author

🎉 💯

@FlorianWendelborn
Copy link

So does this still update the state or did you just implement my suggestion regarding getting rid of the weird return to update functionality?

@jorgebucaran
Copy link
Owner Author

@dodekeract This returns the result of an action to the caller, whatever that may be.

This just helps with @zaceno's use case without breaking the API.

Maybe @zaceno can explain it better?

@zaceno
Copy link
Contributor

zaceno commented Jun 25, 2017

@dodekeract: still updates state. I kind of still wish we could have getters, i.e. same as actions but dont update state -- just return out whatever the inner function returns from the state.

... thinking maybe you'd be more ok with the behavior of actions if there were getters to use for when you don't want the state to update?

@jorgebucaran
Copy link
Owner Author

@zaceno Those are the old effects no? Maybe we can come up with another clever way?

@zaceno
Copy link
Contributor

zaceno commented Jun 25, 2017

@jbucaran -- that's right! Hadn't thought of it before because I always thought of effects as a way of batching and/or deferring reducers -- not as a way of getting/processing data from the state -- but the old effects could work for that too.

I'm open to thinking about & discussing clever ways to get that behavior back. But to be fair you shouldn't give my opinons on hyperapp's state management too much weight, since I've switched my personal projects from away from hyperapp, to picodom + my home made state management solution (which I'm still very much experimenting with)

Anyhow, here are my thoughts:

I feel like we've already "violated" the reducer principle when we allowed actions to return a promise, and in that case not update the state. So we could always take that a step further, with other "magical" ways of returning in actions, that don't cause state updates. Like perhaps if what you returned was an object {update: false, result: x} then the action would return x without updating state.

I grant you, that's not very pretty. I'd much prefer something along the lines of what @dodekeract already suggested, which is that actions always return with the inner-returned value, and don't update state by default. Instead they are passed a setState or similar function to be used inside the action, when you actually want to set the state.

@jorgebucaran
Copy link
Owner Author

@zaceno Thanks for the feedback! Definitely, something to think about.

@FlorianWendelborn
Copy link

FlorianWendelborn commented Jun 26, 2017

without breaking the API — @jbucaran

Not true. In complex actions I'm often using return actions.someOtherAction() to end the action execution and forward it to a different action.

Now, this would update the state twice which may even lead to race conditions or hard to track down bugs.

Actually, I just upgraded from 0.9.2 to 0.9.3 and my whole application is so broken that the state isn't even initialised. I'm getting "can't access something of undefined" in all components, lol.

@jorgebucaran
Copy link
Owner Author

@dodekeract I see what you are saying. I can try to give priority to #90 (leaving #238 for later) and see if that helps, otherwise, we can reverse this.

@jorgebucaran jorgebucaran deleted the feat/actions-return-sub-state branch July 14, 2017 10:40
jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018
Return the state fragment to the caller if the action is a reducer.
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