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

Make m.redraw() purely asynchronous, add m.redraw.sync() #1592

Merged
merged 5 commits into from Jul 17, 2017

Conversation

Projects
None yet
6 participants
@pygy
Copy link
Member

pygy commented Feb 6, 2017

Not sure if it is welcome, but several people in the chat (recently) and elsewhere (less recently) mentioned that they'd prefer always-async redraws...

There they are.

Mounting is still synchronous for both routed and non-routed components (unless the user opts out of synchrony by using onmatch). Setting the route redraws the whole app (otherwise, the redraws when setting the route aren't throttled).

I've introduced a test-utils/throttleMock.js that can be fired manually rather than waiting for the frame delay. It also allows to ensure that the delayed callbacks have fired (see o.afterEach). api/tests/test-mount.js and api/tests/test-mount.js both depend on it. api/tests/test-redraw.js is still mostly timeout-based in order to test the actual throttle code. I've also hardened a few tests.

Edit: I had forgotten to add the throttleMock code...

@pygy pygy force-pushed the pygy:async-redraw branch 2 times, most recently from de70d2b to 521a4fc Feb 6, 2017

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

I like the general idea. I just found a couple oddities

@@ -11,9 +11,14 @@ module.exports = function($window, redrawService) {
var render, component, attrs, currentPath, lastUpdate
var route = function(root, defaultRoute, routes) {
if (root == null) throw new Error("Ensure the DOM element that was passed to `m.route` is not undefined")
var run = function() {
function run() {

This comment has been minimized.

@isiahmeadows

isiahmeadows Feb 8, 2017

Collaborator

Please revert this change. It's just diff noise.

This comment has been minimized.

@pygy

pygy Feb 15, 2017

Member

I use function declarations when the definition not intended to be modified, vs var when it will be re-assigned.

This comment has been minimized.

@isiahmeadows

isiahmeadows Feb 15, 2017

Collaborator

Okay. I see.

module.exports = function() {
var queue = []
return {
throttle: function(fn) {

This comment has been minimized.

@isiahmeadows

isiahmeadows Feb 8, 2017

Collaborator

Is there a particular reason why the below closure is necessary (as opposed to a direct call)? It just seems odd to me.

This comment has been minimized.

@pygy

pygy Feb 15, 2017

Member

You mean having a top level throttleMock() factory?

It allows to have a per-test queue. If a test forgets to clear its queue, it will not have effects on a subsequent test.

I've also added afterEach assertions to ensure that the queues are empty, but problems may arise when writing new tests/specs and the future author hasn't yet added such safeguards, it will not trigger errors in unrelated tests.

This comment has been minimized.

@pygy

pygy Feb 15, 2017

Member

Or do you mean the closure that's returned?

It is to emulate the throttle API. Where a throttled function can be scheduled several times, but runs only once, on fire (which emulates the next frame).

This comment has been minimized.

@isiahmeadows

isiahmeadows Feb 15, 2017

Collaborator

I was referring to the closure.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Feb 15, 2017

@isiahmeadows I had missed your comments, thanks for the review

@isiahmeadows isiahmeadows requested a review from lhorie Feb 15, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Feb 20, 2017

Ping @lhorie (IIRC you had a few objections/concerns?)

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Feb 21, 2017

This should be put on hold until #1643 is merged...

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 3, 2017

Another reason at least to redraw everything on m.route.set rather than only the routed view:

I've added a temporary "table of contents" debug view to navigate an app more easily while it is being built (it is a form with a pre-defined user flow, I want to reach arbitrary pages easily during dev).

I wanted to highlight the active route in the list using m('a' + m.route.params('step') === step ? '.selected':'', {href: '/'+step}, step).

But the mounted component redraws on click before the params are updated, and when the route finishes, only the routed component is updated. As a consequence, the TOC is always one step behind the main view...

Tacking the menu in a fragment at the top of the routed of the view is not an option because the app lives in a lightbox popup (defined outside Mithril) with little room left... An extra redraw isn't an option either So, for now, the current page isn't highligted.

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Mar 4, 2017

@tivac @isiahmeadows @lhorie can anybody provide a reason why this shouldn't be merged?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 4, 2017

For one, @barneycarroll api/tests/mount.js is one gigantic conflict... I rebased it between the time #1652 was squash-merged and the time I realized it shouldn't have been (I actually noticed the squash after looking at the local log post rebase). I may have to redo it...

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Mar 4, 2017

@pygy Apologies...I clicked "merge pull request" this time, so I have no clue what happened. 😖

If necessary, you may want to check this out from a while back on Gitter. That should update your local history without trashing later commits.

@pygy pygy force-pushed the pygy:async-redraw branch 2 times, most recently from 2f29046 to 4218b32 Mar 8, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 8, 2017

@isiahmeadows the re-rebase was actually painless :-)

@lhorie you actively opposed this proposal in the past do you still think there's a use case for a sometimes synchronous redraw API? Maybe we could have m.redraw(true) or m.redraw("sync") that always redraws synchronoulsy whereas m.redraw() is always async?

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Mar 8, 2017

I'm A) in favor of always-async redraw and B) against m.redraw(<val>) for sync redraws.

If it's going to be provided it should be a more explicit API, like m.redraw.sync().

I don't remember & can't easily find the arguments against always-async redraw. 😕

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 8, 2017

I won't dig the post, but Leo mentioned a use case where you want to measure something in the DOM in oncreate and immediately adjust the size of another element (through vDOM) without a flash with the bad size.

He also documented the sometimes-async behavior as a deliberate optimization.

He's been silent on the topic ever since.

👍 for m.redraw.sync() if it were ever needed.

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Mar 8, 2017

IIRC my main concern was that some things only work if they are triggered from a user-created contexts (e.g. video autoplay in ios)

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Mar 8, 2017

Ok, the proposal is:

  • m.redraw() becomes always async
  • m.redraw.sync() is added so sync redraws can be forced when necessary.

This will be a breaking change if so, and require bumping mithril on npm to 2.0.0. Is everyone ok with this? A 👍 reaction to this counts, I think this needs to be unanimous.

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Mar 8, 2017

what is supposed to happen if one calls m.redraw.sync from oninit?

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Mar 8, 2017

what is supposed to happen if one calls m.redraw.sync from oninit?

🔥 🔥 🔥 🔥

I'd expect it to throw, that seems like an impossible request to process.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 9, 2017

Oh, yes, I had forgotten about video auto-play...

what is supposed to happen if one calls m.redraw.sync() from oninit?

We can guard against nested render on a given root.

Sync redraws from oncreate are also problematic. If you mount {view:()=>[m(A), m(B)]} and synchronously redraw from A.oncreate, B.onupdate will fire before B.oncreate.

We may still enable these (by releasing the render lock before the post-redraw hooks are run), with a warning in the docs about proper usage.

Edit: and sorry about the minified file, I'll remove it...

@tivac I just gave my 👍 , but I'm not sure it is breaking. The documentation say:

Typically, m.redraw triggers an asynchronous redraws, but it may trigger synchronously if Mithril detects it's possible to improve performance by doing so (i.e. if no redraw was requested within the last animation frame). You should write code assuming that it always redraws asynchronously.

@pygy pygy force-pushed the pygy:async-redraw branch from 4218b32 to cb44c55 Mar 9, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Mar 9, 2017

Regarding sync redraws from hooks, we may detect nested render calls and, rather than throwing, schedule an extra synchronous render of the same root. We'd have to keep a flag on the root, and stash the vnodes somewhere, though, and be careful about infinite loops... Maybe allow one extra sync render?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Mar 9, 2017

@tivac I wouldn't necessarily call it a major breaking change, considering how it's documented.

Oh, and regarding m.redraw.sync, I'd throw if that node is under the currently rendered tree unless we're running an oncreate or onupdate hook and the node is either the current vnode.dom or a descendant thereof (thus allowing subtree rendering).

The logic might look like this:

function isMountable(root, mounted, vnode) {
    if (!root.contains(mounted)) return true
    if (!vnode._inMutatingHook) return false
    if (vnode.dom === mounted) return true
    if (vnode.dom.contains(mounted)) return true
    return false
}

@pygy pygy force-pushed the pygy:async-redraw branch from e5e2ce2 to b004c20 Jun 13, 2017

@pygy pygy changed the title Make m.redraw() purely asynchronous Make m.redraw() purely asynchronous, add m.redraw.sync() Jun 13, 2017

@pygy pygy force-pushed the pygy:async-redraw branch from 6423a12 to 7de0124 Jun 13, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 13, 2017

I've added m.redraw.sync() as well now, but no m.redraw.safe() yet. Just returning !rendering would not cut it for the first render performed at mount time...

No docs at this point.

Edit: Before this is merged, we may want to cut v1.1.2 with the latest bug fixes.

@pygy

This comment has been minimized.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 13, 2017

I hadn't run npm install, therefore the linter wasn't running...

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jun 14, 2017

@pygy I'd be okay with cutting 1.1.2 first. (It's long overdue, anyways.) This is borderline breaking, so it's probably best to postpone.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Jun 14, 2017

Agreed on a 1.1.2 first. I still think this should be 2.0.0 (because major version numbers are cheap) but would acquiesce to a 1.2.0 instead.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 14, 2017

I'd like to merge the #1734 fix before we cut 1.1.2 (the fix is easy, but the test suite only covers pushState-based routing, and the fix breaks all the routing tests, I'm currently sorting things out).

Re. 2.0, why not... There are other potentially breaking changes in the pipeline in peripheral APIs like m.request, the attrs changes, ...

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 17, 2017

@isiahmeadows @tivac @lhorie I'd like to merge this now. I've added a v1_1_x branch for merging backwards compatible fixes while we work on v2.0

@pygy pygy merged commit 8ab3179 into MithrilJS:next Jul 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 18, 2017

I only realize that I forgot to update the docs about this... I've started some work in redraw.md and mount.md, I still have to update route.md, changelog.md and perhaps autoredraw.md

pygy added a commit to pygy/mithril.js that referenced this pull request Jul 19, 2017

pygy added a commit that referenced this pull request Jul 19, 2017

@andraaspar

This comment has been minimized.

Copy link
Contributor

andraaspar commented Aug 23, 2017

Sorry for being late to the game. I think the sync bit is totally unnecessary.

I ran into a situation recently where I was very surprised that lifecycle events may be called out of order – and I think the sync does just that. This happens when you call m.redraw() (now m.redraw.sync()) from a lifecycle callback, like oncreate. What happens is that oncreate and onupdate and onremove has not been called on all components, yet a new render cycle begins. This makes it insanely hard to reason about lifecycle calls, and results in a really weird set of bugs. I'd been scratching my head for a day. I think this should be avoided at all costs, and it can be.

What I came up with was this:

import * as m from 'mithril'
import { model } from '../client-statics'

const queue: string[] = []

export function redraw(reason: string) {
	if (model.isRedrawing) {
		queue.push(reason)
	} else {
		model.isRedrawing = true
		model.redrawReason = reason
		m.redraw()
	}
}

export function redrawDone() {
	model.isRedrawing = false
	if (queue.length) {
		redraw(queue.shift())
	}
}

Then in AppComp:

...
export class AppComp {
	onbeforeupdate(v: Vnode, o: VnodeDOM) {
		console.log(`--- Redraw: ${model.redrawReason} ---`)
		model.redrawReason = undefined
	}
	view(v: Vnode) {
		return (
			<div>
				...
				<div
					onupdate={() => {
						console.log(`--- Redraw done ---`)
						redrawDone()
					}}
				></div>
			</div>
		)
	}
}

This made sure that redraws follow each other immediately, without a flash, and yet they do not get nested. This takes care of what @lhorie was worried about, but it needs to be implemented in Mithril, or it won't work for multiple components mounted in a HTML.

The thing that we need to be able to queue operations after the redraw is a Promise (see #1837). Please. I'm sure Mithril knows when it's done redrawing. Why not resolve a single Promise then? You could queue Promises as seen with redrawReasons above.

@andraaspar

This comment has been minimized.

Copy link
Contributor

andraaspar commented Aug 24, 2017

I am putting a PR together to demonstrate how this can be achieved, but I am finding that promises are suboptimal for the task, as the then would fire after the redraw cycle. Maybe callbacks would be better.

@andraaspar

This comment has been minimized.

Copy link
Contributor

andraaspar commented Aug 24, 2017

@andraaspar

This comment has been minimized.

Copy link
Contributor

andraaspar commented Aug 24, 2017

OK, here's the callback version.

It's way less invasive and much more predictable than the Promise version.

@lhorie @pygy @isiahmeadows @tivac Would you like to review it?

@andraaspar

This comment has been minimized.

Copy link
Contributor

andraaspar commented Aug 24, 2017

Slightly modified my views on this, and submitted a PR: #1946

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment