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

Access current state in onremove? #242

Closed
zaceno opened this issue Jun 22, 2017 · 34 comments
Closed

Access current state in onremove? #242

zaceno opened this issue Jun 22, 2017 · 34 comments
Labels
discussion enhancement New feature or request

Comments

@zaceno
Copy link
Contributor

zaceno commented Jun 22, 2017

For the transitions solution I've got to work in hyperapp-apps, there needs to be a way to access the state of the app when onremove lifecycle methods are called. Not the state that was when they were bound.

The onremove handler can call actions. Actions will have access to the state as it is at the time they are called. But you can't get anything out of them.

There are three solutions I can see:

A) Either the signature for on remove handlers is augmented to look like: ... onremove={(el, doRemove, state) => {...}}

or B) We can add the concept of getters alongside actions. Getters would work just like actions, except that what they return goes back out to the caller, instead of modifying the state

or C) Along the lines of what @dodekeract has suggested before: We change the behavior of actions to return whatever they return without modifying the state, and give them access to a "setState" or whatever function they can use to perform state mutations.

From what I understand, the API is pretty much locked down so I don't have much hope for B or C. A could happen but it's the least elegant / general. It's very specific to my problem.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

Another, more serious problem with (A), is that it would couple the state management to the virtual-dom solutions within hyperapp. And make picodom diverge from hyperapp's vdom. That would not be good.

@jorgebucaran
Copy link
Owner

@zaceno C) would not be so elegant either.

There's also...

D) Pass the emitter into views so you can create your own magic events.

@jorgebucaran jorgebucaran added discussion enhancement New feature or request labels Jun 22, 2017
@SkaterDad
Copy link
Contributor

Option C would really make hyperapp less appealing, in my opinion.

Does this need to be baked in? We can already access (state, actions) in the view code, so you can pass it to your lifecycle hooks through higher order functions.

Or does this not work how it appears due to how hyperapp updates the state?

const onRemoveHandler = (state, actions) => (element, done) => {
   console.log(state)
}

app({
  state: { value:  1 },
  actions: {},
  view: (state, actions) => h('div', { onremove: onRemoveHandler(state, actions) }, 'Test')
})

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

Personally I would prefer option B, but again, I've got no hope this will actually make it in. Just leaving the ticket here for the record.

Correct, your example won't work... or rather: it should not work according to the design, but it will work only because, as hyperapp happens to be implented for now, the reference to the state object does not change through updates.

The reason it shouldn't work is because the onremove handler that gets called is never updated with the latest state, because it's not supposed to be rendered in the dom (it's supposed to be removed!)

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

Try your same example with a primitive as the state rather than an object, to demonstrate that it doesn't work.

@SkaterDad
Copy link
Contributor

Try your same example with a primitive as the state rather than an object, to demonstrate that it doesn't work.

Yeah, that makes sense. I made my example state and object for that very reason, but was still unsure if hyperapp's state updates would change that reference for the sake of immutability or something. For now, I'm glad it works since I made need similar tricks for something I'm toying with.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

Pass the emitter into views so you can create your own magic events

@jbucaran that's intriguing. Don't we already do that? But, I'm not exactly sure how I could use that to get the current state in onremove handlers. Could you elaborate on how the "magic events" could help?

@jorgebucaran
Copy link
Owner

@zaceno Don't we already do that?

Nope. Views have this signature: (state, actions).

Events have the signature (state, actions, data, emit), so you could use it to get a new copy of the state. But I still don't understand why the state bound to the view is not the same as the current state when the onremove handler is called. 🤔

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

I'll put together a demo example explaining it but basically: if a node gets removed, it means it's not part of the Vtree currently being rendered. Which means we are not redeclaring the onremove handler with the new state. If an onremove handler accesses state, it is accessin the state it was bound to (by closure) the last time it's node was rendered.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

Another, perhaps better way to explain it: when we call onremove in the patch-function, it is always for vnodes of the OLD tree. The old tree has not been touched since back when it was a new tree. that means that the onremove handler has the same closure as it did when it was a new tree. That means old state, unless there is a function in the closure we can call, that always returns the current state.

@jorgebucaran
Copy link
Owner

@zaceno Why do you need to access the state inside onremove?

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

As you may recall, I'm using the oncreate and onremove handlers to attach css classes to the element, in order to effect entry and exit transitions.

Now say you have something like a "carousel" or a slideshow you want to navigate forward or back with buttons to the left or right. If you keep track of the current and previous slides (as well as the order of the slides) in the state, you can deduce wether the current slide should come in from the left (you clicked the back button) or the right (you clicked the forward button). That's easy to take care of in the oncreate handler because it is called just after it is defined. But the old slide should correspondingly exit to the left or right. But using the state it has access to will (or should, rather) make it leave according to whatever we clicked the previous time.

So that's basically why. I need to decide what to do in the onremove handler, based on the current, not previous state

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 23, 2017

@zaceno Gotcha. What would be the ideal way to do what you need that doesn't need the state to be passed to onremove?

Maybe what we need new events?

EDIT: Added more stuff.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

The best (and to my knowledge only other) way would be if there was a function available to onremove handlers, which is bound to always access the current state. Actions are exactly that - except since they don't return anything, I can't use them to get access to the state. Hence my suggestions B and C

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

Again I'm not sure how events help. Maybe I'm just slow tonight... :-P

@jorgebucaran
Copy link
Owner

@zaceno What about convenient getState action?

jorgebucaran pushed a commit that referenced this issue Jun 23, 2017
Now actions.myAction will return whatever the original myAction function
returns. Before, this was only true if myAction returned a Promise.

Fix #242.
@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

@jbucaran: awesome! That would totally work. A nice and general solution.

Wonder if there are other ways to exploit this "actions return the new state" behavior... 🤔

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 23, 2017

@zaceno There is the horrible, not recommended way:

actions: {
   foo: () => ({
       bar: "baz",
       then: { /* fake promise */ }
   })
}

There's always the old way of wrapping your stuff in a promise, which again, is not recommended as I still believe that actions should be used only to update the state, not to pass data around.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

Hehe yeah that's sneaky ;)

@zaceno
Copy link
Contributor Author

zaceno commented Jun 23, 2017

Dang.. using getState() defined in that way breaks stuff. I can't say exactly where or how yet (I've been testing on codepen without source maps). But I suspect the reason is somehow because the action -- even though it is defined as state => state will trigger an update and rerender when called.

Here's the codepen. Find the line "UNCOMMENT LINE BELOW" ... and try it out ;)

https://codepen.io/zaceno/pen/xrrWdK?editors=0011

@zaceno zaceno reopened this Jun 23, 2017
@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 24, 2017

@zaceno How does it break stuff? Infinite loop? Yes, the getState example provided will cause a redraw.

EDIT: I couldn't run the example, something was missing.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

Like I said, haven't had time to debug it with source maps - but what I get isn't a "maximum call stack exceeded" type error, but something in the patch is undefined. still it's surely about the recursion somehow.

Yeah I noticed that something isn't working now too. I'm guessing my file server (serving the build of master) died. Will look into it.

@MatejBransky
Copy link

I think that this could be easily solved similar way as React does (React transitions) if we find solution for components in HyperApp. #238

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

@jbucaran: never mind my example. I realize now it can't work. Either we need a way to return stuff from actions in such a way that they don't cause another render. Or if we used requestAnimationFrame or similar to defer all renders, so we can avoid recursively rendering.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

@matejmazur: That could well be so, but I'm doubtful. There are more fundamental differences between hyperapp and React. Like the requestAnimationFrame thing that prevents recursively rendering. Either way, I'm interested to see what you come up with.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

Perhaps the best for now is just to return an already resolved promise from the getState action. That way at least the rendering won't recurse

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 24, 2017

@zaceno I thought you only needed to look at the state inside onremove. What else are you using onremove for? 🤔

EDIT: Does running getAction inside onremove to be called again?

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

ah it could be an effect of me trying to not repeat myself. Running getState in onremove will probably not make onremove get called twice, and since onremove is deferred, that should be safe. The problem Is probably that I'm using the same method (with getState), to determine direction for the entry transition as well (in oncreate). That will surely cause a mess, but in oncreate I can just use the state passed to the view. I don't need an action for that. Going to try that...

(Sorry for messy writing - typing on my phone)

@zaceno
Copy link
Contributor Author

zaceno commented Jun 24, 2017

Yup, that was the problem. Not onremove, but me using the getState action in oncreate (trying to save some typing). This works now: https://codepen.io/zaceno/pen/xrrWdK?editors=0010

...but, for the time being, I'm using build of hyperapps master hosted on my personal server where I had to use a self signed ssl cert in order for codepen to use it. So if the codepen doesn't work, go to: https://zach.enochsson.net/hyperapp.js, and accept my certificate (if you're comfortable doing that of course) -- then the codepen should work.

So we have a s olution. A bit messier than I'd hoped I guess, but it is after all probably something of an edge case, so that's ok. Going to close now.

@zaceno zaceno closed this as completed Jun 24, 2017
@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 25, 2017

@zaceno Thanks! After components, batching renders with requestAnimationFrame #90 will be (hopefully) the last thing todo before 1.0.

@FlorianWendelborn
Copy link

@zaceno

self signed SSL cert

Why don't you use letsencrypt, either directly or via caddy?

@zaceno
Copy link
Contributor Author

zaceno commented Jun 25, 2017

@dodekeract: hadn't heard of letsencrypt before. How widespread is the trust of this CA?

EDIT: not finding any references to them in my system (Mac OS Sierra 10.12.3)

@FlorianWendelborn
Copy link

@zaceno They're supported everywhere that matters. It's definitely supported on macOS Sierra 10.12.5. They're not a "root CA", so you won't find them directly. They use a well-established trust chain. Just try them out, they're everywhere if you look closely.

I'm personally using them for all my SSL certs, including my mail server, and I never had any issue whatsoever with it.

@lukejacksonn
Copy link
Contributor

letsencrypt is a legitimate solution provider.

If you are just trying to host your own projects easily, over https then why not use gh-pages? If you want custom domains just include a CNAME in your repo (and point your domain at the github pages server, via cloudflare for SSL), it sounds like a hassle but once setup it just works.. and is free.

I wrote a little repo/guide on how to setup deploys.

jorgebucaran added a commit that referenced this issue Jan 7, 2018
Now actions.myAction will return whatever the original myAction function
returns. Before, this was only true if myAction returned a Promise.

Fix #242.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants