charypar
commented
9 years ago So, my thoughts (apart from bugfixes):
Cursor changes
-
Cursor.updatemight as well return a new instance, although I don't really know why. In my mind that call has no return value. Cursor.on-changeshould need to pass in a cursor, because to register the handler you already have it. If you need the cursor instance in the handler for... reasons, you can achieve the same thing with closure / currying. So unless I'm missing some use-case, I'd rather not do this. If the reason is performance, then I'd rather remove the argument completely and rely on closure all the time. I'm not entirely sure what the current semantics are, whether we call on-change before or after changing the backing ref.Cursor._swapwas needed as the only place where the backing data can get replaced. The point of that was to have a controlled way for the backing reference to update so that we can check invariants and stuff. The method is internal anyway. ... I don't see howCursor.updateis related, it relies (should reliy) on_swapto operate.
I'm not sure right now and need to test this, but I think your changes, which create a new root cursor every time an update is made will break things, because people can still hold instances of the old root or any of its subcursors derived from it. And the old root still holds the old data and old listeners array. It's ok to create new subcursors on each get, but they all need to share the same root. You essentially split the cursor in two with each update, which is semantically wrong. In your version, the cursor is just an extra layer around the immutable data structure - a mutation creates a new thing, like it would with the immutable. All previously created cursors and sub-cursors won't see the update which is the basic promise of the cursor ("You will see the current state of the top level ref whenever you deref").
Application changes
I do agree that application needs refactoring, but hold off on it untill the server side stuff is done. I promise the split methods will eventually make sense.
TL;DR
... can you open a new one with just hot loading and bug fixes, I guess? :) The pure render mixin will work the same way if you compare cursors by their .raw! instead of a reference check.
tabazevedo
Breakdown of changes.
Built on top of https://github.com/redbadger/reflex/pull/16
Bug Fixes
Cursor change breakdown
API changes
Cursor.update(updater(current-value))now returns a new cursor with the updated data structure instantly as well as notifying listeners (still gives you derefed value to the updater function).Cursor.on-change(callback(new-cursor))now fires callbacks with a cursor rather than dereferenced value.Cursor.transform(a(b), updater(current-value))- new function. takes a conditionalaand applies an updater functionbto the cursor untilais fulfilled (checking if a returns true when passing the value ofbinto it. Only notifies listeners once updates are finished.Cursor.swap(n)- function removed. why not just useCursor.update -> new-data?Functionality changes
Cursors no longer swap their own data by default. A cursor pointing at a data structure will return the same data structure regardless of what you do to it. Updaters on a cursor will always return a new cursor pointing to the newly updated data structure. This allows you to propagate cursors down your application as props, so you don't have to
forceUpdate()and render the entire application each time a change is detected.Application change breakdown
Application state now lives outside the component, instead rerendering it with the global state existing as props.
Application.start and Application.renderwere merged into one function (they looked REALLY similar except the final part) which returns either a string or renders the application based on if we're server or client-side.Commented it for clarity when reviewing.