Allow the use of Promise that resolve to a state #305
Conversation
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
+ Coverage 99.35% 99.37% +0.02%
==========================================
Files 2 2
Lines 154 160 +6
Branches 48 51 +3
==========================================
+ Hits 153 159 +6
Misses 1 1
Continue to review full report at Codecov.
|
src/app.js
Outdated
| if (result != null && typeof result.then == "function") { | ||
| result.then(update) | ||
| } else { | ||
| update(result) | ||
| } |
There was a problem hiding this comment.
Let's change this block to:
if (result != null && typeof result.then === "function") {
result.then(update)
} else {
update(result)
}and move the update implementation before the implementation of repaint right after return emit, around like 35.
Rewrite it like this please.
function update(withState) {
if (withState != null) {
repaint((state = merge(state, emit("update", withState))))
}
}There was a problem hiding this comment.
According to the slack discussion, we move from result.then != null to typeof result.then == "function because :
At least test if then is a function ?
If the function return nothing, theresult != nullwill silent ignore it. But if then is not a function, that will break the thing
There was a problem hiding this comment.
@Swizz That's probably a misunderstanding. If then is not a function then the user is doing something very weird.
There was a problem hiding this comment.
Can we forbid the use of then in state ?
app({
state: {
foo: 0,
bar: 1,
then: 2
},
actions: {
change: state => ({ then: state.then + 1 }),
}
})There was a problem hiding this comment.
I don't think we can forbid it. I am not too worried about this.
There was a problem hiding this comment.
So, we need to check if resul.then is a function in order to call it.
If the end user return a function let execute it, and the result not null check will do the rest. If this is not a function lets do a regular result patch.
Without that, using then in result will break the app with an hyperapp internal error.
There was a problem hiding this comment.
I don't understand what you are saying, but yes, use:
if (result != null && typeof result.then === "function") {There was a problem hiding this comment.
If I use the following app
app({
state: {
foo: 0,
bar: 1,
then: 2
},
actions: {
change: state => ({ then: state.then + 1 }),
}
})This will raise an error when using action
Uncaught TypeError: result.then is not a function
There was a problem hiding this comment.
if (result != null && typeof result.then === "function") {
|
So this means you can no longer chain async actions, without the state being changed in each step of the chain? I have a vague feeling (can't substantiate it though...) that while this will make some things that are already possible simpler, it will also make other things (that we can do today) impossible, or more complicated. Maybe I'm just an old man afraid of new things... ;) |
What do you mean? |
|
See this example: const delay = seconds => new Promise(done => setTimeout(done, seconds * 1000))
app({
state: 0,
view: (state, actions) =>
<main>
<h1>
{state}
</h1>
<button onclick={actions.upLater}>+</button>
</main>,
actions: {
up: state => state + 1,
upLater: async (state, actions) => {
await delay(1)
actions.up()
}
}
})With or without this patch, if you click ten times the Now, see the following example: app({
state: 0,
view: (state, actions) =>
<main>
<h1>
{state}
</h1>
<button onclick={actions.upLater}>+1</button>
</main>,
actions: {
upLater: async (state, actions) => {
await delay(1)
return state + 1
}
}
})Without this patch the above is an error, but with this patch, actions.upLater are debounced, so clicking the |
|
@jbucaran Well I guess what becomes impossible with this change, is if you wanted to get inbetween an xhr call returning, and the state being update, but you didn't want to put the logic for that in a I often find myself using three distinct "types" of actions:
Mainly I think my worry stems from this new behavior breaking this pattern (since now, async-backend-request-actions will also update state. At some later time...). I don't yet see any real practical problems, though, so I'm probably just stuck in my ways, uncomfortable with change. I do see how this makes actions more elegant. It reduces my "three-type" pattern down to two types:
I can feel my opinion gradually becoming more positive to this change during the writing of this post ;) |
|
How would |
|
Unfortunaly using const p = new Promise(function(resolve, reject) {
reject('fail');
});
p.catch(function(e) {
console.log('error', e);
}).then(function(value) {
console.log('success', value);
})
It would be possible to const emit = app({
actions: {
issues() {
const request = fetch("https://api.github.com/repos/hyperapp/hyperapp/issues")
.then(response => response.json())
.then(issues => ({ issues }) )
request.catch(error => emit('error', error))
return request
}
}
})with async/await const emit = app({
actions: {
async issues() {
try {
const response = await fetch("https://api.github.com/repos/hyperapp/hyperapp/issues")
const issues = await response.json()
return { issues }
}
catch {
emit('error', error)
}
}
}
}) |
|
I am wayyyy late to the conversation here (since just this afternoon even!) but I remember suggesting this a long while back.. declined due to having to support This would be how I would Allow the use of Promise that resolve to a state. function initialize(namespace, children, lastName) {
Object.keys(children || []).map(function(key) {
var action = children[key]
var name = lastName ? lastName + "." + key : key
var update = (action, name) => data =>
action(
state,
actions,
emit("action", {
name: name,
data: data
}).data
)
if (typeof action === "function") {
namespace[key] = data =>
Promise.resolve(update(action, name)(data)).then(result =>
result != null && repaint((state = merge(state, emit("update", result))))
)
} else {
initialize(namespace[key] || (namespace[key] = {}), action, name)
}
})
}This means that every action is the promise of an update (or noop if the return value is null). If the action is sync (returns a primitive value or null) then it will resolve immediately and trigger an update. If the action is async then it waits for the promise chain to resolve.. if the resolved value is not null then an update is triggered. I actually managed to dig out our old conversations on such matters:
Not sure if this is exactly what you guys are trying to solve but it sure looks/sounds similar so might be worth some referencing! |
|
@zaceno Can you share exactly what kind of code would become impossible after this change? I still can't picture what you are saying.
This patch only modifies the behavior when your return the promise. If you don't return anything, nothing happens. |
|
@lukejacksonn The approach you suggest means we need to put Promise inside core, which is another issue on its own, but what's the difference between your suggestion and @Swizz's patch? |
|
Maybe it is too premature to merge this in. I can see myself using async actions this way, but I still don't know what the cons / cost of this new feature would be? 🤔 |
Yes it does 😌 bye bye IE11.. indeed another issue on it's own. My only argument for this is that there is nothing cool about supporting IE11 out of the box. Support could be achieved through a curated
It takes advantage of a standardised platform feature rather than a proprietary implementation. Less upkeep and no weird limitations/workarounds like this. What @zaceno wrote in #305 (comment) describes exactly how I have found myself using actions and highlights the paradigmatic repercussions perfectly 💯👏
TLDR; I am tempted but undecided |
|
@lukejacksonn Thanks! I am still unclear about the cons myself. Is #305 (comment) a desirable behavior? @Swizz If you can answer some of our questions, you'll get a better chance to have this merged! 😄 |
|
I will try to do my best, but keep in mind this is my implementation regarding to our decision to avoid Promise in core.
Lot of people is still using Bluebird and similar, Using then that way, would it be a better choice regarding exotic Promise implementation ? IDK both are great for me
I dont understand very well the main concern here. I understand the fact we are moving from "An action that will do the request and the action that will update the state" to "An action that will make the request and update the state", but the first one is still possible. This is still possible to call an another sync action and resolve the async one to
Yes, the Promise API, in general, is designed to be used like this. At each async step, you are waiting the new step to resolve
IDK. Maybe yes, Maybe not. Here, we are just assuming the resolved result of an async action is nothing more than the result of a sync action. The later update the state now and the first one update the state when resolved. This is the main purpose of async/await be like a sync routine but with async stuff.
Async/await is 95% Promise and return a Promise. So I assume : Yes.
Now, Promise and Callback are very similar in term of perf. And here we are already supporting Promise/Async actions. I am just wondering : Why not resolving to an update of the state directly like a sync action ? |
|
@zaceno mentioned something about "inbetween an xhr call returning", but without a code example I can't picture it. |
|
I am not Swiss, but.. I have no many experiences with hyperapp to figure out. Firstly, the one who use async actions that return something ? But this is totally a nonsense regarding actual API. Async which return nothing will be ignored as well and this is the current pattern, so ? |
Sorry, lol 😅 Well, let's wait for @zaceno to chime in. |
|
Do I need to work on your requested changes or may I wait ? |
|
I like the idea behind this PR. 👍 Half the time I forget that I can't do this already, so to me this type of change would make Regarding just using Promises in core... I'm not a fan of that idea. One of my main uses for |
|
@Swizz @jbucaran Sorry for the delay. Don't hold anything up on account of me. I've changed my mind (or rather, realized my concerns were unfounded) and give this new behavior 👍 (@jbucaran I'd give you a code example of what I was talking about, but there's no point because I can't see any way it would be a problem) |
|
@SkaterDad Larger with some bytes. I dont know why bundlesize didnt work on this PR, so I cant tell. @zaceno I am glad to read this 👍 🎉 Alright, I will work on request changes later ❤️ |
|
@Swizz I know It does seem strange that |
Please do 🙏 |
|
I'll review how this affects @attach-live's codebase today. CC @fvj |
|
@jbucaran @zaceno @lukejacksonn @SkaterDad To be Promise compliant without the need of Promise in core, the only thing we do is to be Promise/A+ compliant. And one of the big thing that define a Promise is :
Here is our case. We are relying on the So this patch seem compliant to Promise without the need of the Promise keyword in source. If you want to go further, we can also be compliant with : .all(promises: Array<Promise>).then(onFulfilled: (values: Array<any>) => any)To be aware of actions which return an array of Promise. And maybe we can go to be ES Observable compliant too .subscribe(obs: Observer)Thinking too loud. But I am an FPR Lover after all.const { h, app } = hyperapp
const { periodic } = most
/** @jsx h */
app({
state: { ho: 0, mi: 0, se: 0},
view: ({ho,mi,se}, actions) => (
<main>
<h1>{ho>=10?ho:'0'+ho}:{mi>=10?mi:'0'+mi}:{se>=10?se:'0'+se}</h1>
</main>
),
actions: {
clock: (state, actions) => {
return periodic(1000).
scan(count => count + 1, 0).
map(count => ({
ho: ~~(count / 3600),
mi: ~~(count % 3600 / 60),
se: ~~(count % 3600 % 60)
})
}
},
events: {
loaded: (state, actions) => {
actions.clock()
}
}
})Each seconds, a new value is emitted in the stream sequence, state subscribe to the returned stream, and will be updated by the value emitted in the stream. (I am joking, or not IDK 🤔) |
|
@ngryman When you have time, please share your feedback! 🙏 |
There was a problem hiding this comment.
@Swizz Please rebase and update with the changes we talked about here:
There was a problem hiding this comment.
@jbucaran Looks like @attach-live will be fine with these changes. Some minor changes are needed, but overall I think this might even lead to simplified code. Would be nice if @fvj could confirm this for his projects.
That being said, I'd still like to have a way to return from actions without updating the state. Perhaps something similar to return {$: false} (some magic key that is unlikely to be a coincidence) would be good to solve this? That way we'd have sane defaults but also allow advanced functionality.
Also, what will happen with throw 'something'?
|
I'm very late on this one. Just dropping an idea for async stuff, why not supporting This has the advantage to be super easy to implement and gives much more freedom to the user: For example: app({
state: {
error: '',
issues: []
},
actions: {
fetch: () => (update) => {
const request = fetch('domain.tld')
.then(response => update({ issues: response.json() }))
.catch(error => update({ error })
}
}
})We just need to expose an |
|
@ngryman Wouldn't this behave the same as |
|
@ngryman Not a bad idea, but the current approach is cleaner and feels more natural. Also, I am saving returning functions for a bit later, since this is essentially how I plan to support getters using a Getter mixin. /cc @dodekeract |
|
@dodekeract In all likelihood, if an exception is thrown, the Promise attempt to call the @jbucaran I tried to work on all my todolist yesterday but tiredness wins. May be during saturday night. @ngryman I admit, This action is sync : updateState(state, actions) {
/* ... */
return data
}This ones are async : async updateState(state, actions) {
/* ... */
return await data
}updateState(state, actions) {
/* ... */
return Promise.resolve(data)
}Nothing more. Your actions return a partial state or a promise that resolve to a partial state. Pretty easy. |
|
@dodekeract Yes. @jbucaran It does not feel more natural for me, but I understand your point. @Swizz I get it, totally. What I'm discussing here is the constraint of having to use the They offer the ability to do whatever you want, and if you want some syntax sugar, it's quite easy to implement in userland. For example we could have some sort of // This would iterate over all actions, for the sake of the example
// and because I'm lazy, I just show what it's doing per action
const thunkify = (action) => (...args) => async (update) => {
const res = await Promise.resolve(action(...args))
update(res)
}
const actions = {
updateState: async (state, actions) => Promise.resolve(data)
}
app({
actions: thunkify(actions)
}) |
|
@ngryman I just want to address a part of your reply.
Nope. We are only making it easier for people already using promises (which I bet are more than those using any other paradigm, but that's besides the point). Now, we are not making it worse for people using good ol' callbacks. app({
state: 0,
view: (state, actions) =>
<main>
<h1>{state}</h1>
<button onclick={actions.upLater}>UP 1s later</button>
</main>,
actions: {
upLater: (state, actions) => {
setTimeout(actions.up, 1000)
},
up: state => state + 1
}
}) |
|
@ngryman I think we can still consider thunks, though. That is, if we can find a way to make the concept more approachable. For example, not using that horrible word anywhere in the docs to begin with. Can you also show an example implementation of app.js? You said:
Please show me. |
|
As said by @jbucaran here, we are not changing the old way to do. We are making possible to move from the following pattern actions: {
setUser(state, actions, user) {
return { user }
},
async handleUser(state, actions, id) {
const user = await fetch(id)
actions.setUser(user)
}
}To this one actions: {
async handleUser(state, actions, id) {
const user = await fetch(id)
return { user }
}
}But the first one, is still possible. You can easily keep a sync action that will be responsible of updating the state and the async that call the sync one after its job will be done. The main goal is only to sanitize the async action api to look the same as sync one. We can easily drop out the need to call an action that only be a setter. EDIT: Run |
I didn't say that :) I said that with this patch I will still have to:
Here companion action being
function update(data) {
if (data != null) {
repaint((state = merge(state, emit("update", data))))
}
}
if (result != null && typeof result == "function") {
result(update)
}
else {
update(result)
}@Swizz Yup, again I totally 100% get the vision you have. I'm just confronting it with other use cases and the fact that this is an opinionated vision. |
|
@Swizz gave me an idea: why not implementing both? That's so easy, and we would:
if (result != null && typeof result.then == "function") {
result.then(update)
}
else if (result != null && typeof result == "function") {
result(update)
}
else {
update(result)
} |
|
The .call is needed because of promises 😞. This will also end up binding the "thunk" to itself, probably harmless. |
|
In order to implement getters, the action must |
|
@jbucaran I didnt understand your last comment, can you provide an example ? |
if (result == null) {
} else if (typeof result == "function") {
result = result(update)
} else if (typeof result.then == "function") {
result.then(update)
} else {
update(result)
}
return result |
|
This will not be compatible with your bytes safety example ? if (
result != null &&
typeof (data = result.then || result) === "function"
) {
result = data.call(result, update)
} else {
update(result)
}
return result |
|
But, that would be equivalent to: result = result.then(update)
...
return resultI thought we wanted to return the promise not the @Swizz Does it even make any difference here? EDIT: Unfortunately, it does make a difference. |
| result = result(update) | ||
| } else if (typeof result.then == "function") { | ||
| result.then(update) | ||
| } else { |
There was a problem hiding this comment.
Could we make this else if (result != null) and then remove the if (result == null) above?
There was a problem hiding this comment.
Oh I see, I think you were discussing a smaller impl above. Disregard.
|
@Swizz 🎉 |
Thunks allow users to return a function from an action, then we will call such function passing the same update function we use enqueue repaints/renders. When using a thunk, it's now up to the user to initiate an update or not.
The following change follow the slack discussion regarding the feature allowing an action to return a Promise that resolve to a state.
Usage :
or
When an action return a Promise. The main argument of the last
.thenclause will be used exactly like a sync action return.