-
Notifications
You must be signed in to change notification settings - Fork 780
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
Collapse reducers and effects into actions. Implement #126. #127
Conversation
If your action returns a model or part of a model, then this value is merged with the model and the view is updated, this is exactly how reducers work right now. If your action returns undefined (does not return anything) we don't update the model, and it works exactly how effects do right now. As a special bonus, if your function returns a Promise-like object, we assume it's an effect too and return the promise. This lets you use async/await with actions or create a chain of task promises. Only remember to always keep your promises.
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 174 171 -3
=====================================
- Hits 174 171 -3
Continue to review full report at Codecov.
|
@dodekeract Review please 🙏. |
Have you seen my proposal here. I have only just skimmed this PR but I think we are trying to achieve the same thing. |
subscriptions[i](model, actions, onError) | ||
} | ||
}) | ||
|
||
function load(fn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just looked the MDN api https://developer.mozilla.org/zh-CN/docs/Web/API/Document/readyState。
How about change this function to this way:
function load(fn) {
document.onreadystatechange = function () {
if (document.readyState == "complete") {
fn();
}
}
}
or better, I want to create a start function contains these code:
function () {
root = app.root || document.body.appendChild(document.createElement("div"))
render(model, view)
for (var i = 0; i < subscriptions.length; i++) {
subscriptions[i](model, actions, onError)
}
}
then you can make the load function into this:
function load() {
document.onreadystatechange = function () {
if (document.readyState == "complete") {
start();
}
}
}
Does it look more clear to read?
(actually this is the first time I do the "review code" thing. thanks this warm community! )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukejacksonn Thanks. I answered some of your comments there, please have a look! For the most up to date info, see the top of this thread. Correct me if I am wrong, but your proposal requires actions to always return a Promise? |
I'd like to proceed with this PR 🔥. Please if you have any further comments, post them soon 🙏 🙏. |
I think I am up to date with updates haha!
I'm assuming you are saying this because of Seeing as action used to return nothing (or more recently actions which could be done here but there would be no point). The only issue I see with it is what @selfup mentioned which is..
I have no solid evidence but I have read that resolving a primitive value is evaluated as container[key] = function (data) {
for (i = 0; i < hooks.onAction.length; i++) {
hooks.onAction[i](name, data)
}
return Promise.resolve(app.actions[key](model, data, actions))
.then(result => {
if(typeof result === 'object') { // Check it wasn't just an effect
for (i = 0; i < hooks.onUpdate.length; i++) {
hooks.onUpdate[i](model, result, data)
}
model = merge(model, result)
render(model, view)
}
})
} It also has the benefit of making all |
@lukejacksonn Ah, okay, right. In that case, I don't see what prevents you from doing this already then. |
@lukejacksonn It seems you were suggesting to use Promise directly inside hyperapp? |
@lukejacksonn Even if promises were super fast, which they are not, the problem with this is forcing users to polyfill |
I wasn't aware that was a limitation of the project. Most developers are very accustomed to importing babel-polyfill these days especially if using a bundler, it would mean having to serve an IE friendly version on CDN though perhaps. Performance is a real issue but I would be intrigued to see how it compares. I don't think there is going to be much in it given the simplicity of Let's get this PR merged and I can play around 👍 |
@liadbiz I've added a few comments to explain what's going on in certain places. If there are other places you think need further explanation let me know. |
@lukejacksonn Thanks, for looking into perf. See this: https://github.com/lukeed/polkadot/blob/master/benchmarks/readme.md by @lukeed.
The project has a determined goal of not straying too far away from the vicinity of 1kb. If we introduce Well, not true, if you are using an ultra modern browser, you wouldn't need a polyfill, so technically we wouldn't be breaking any promises, but right now, we can support IE10 out of the box without polyfills and still be 1kb, which is nice. The entire code base is a mix of mostly ES3 and some ES5 (forEach, Object.keys) and I've been really trying to keep it that way. Not everyone is using promises. Whatever their reasons are, if we use |
Collapse reducers and effects into actions. Implement #126.
Remove reducers/effects for a familiar concept: actions.
If you don't know Promises work or don't want to use them, that's okay. Promises are not necessary in order to use hyperapp, but it's nice you could use them if you wanted and hyperapp does the right thing (which is returning them to the caller instead of updating the model/view).
Caveats
Related: