MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
13.99k stars 926 forks source link

How do I opt out of redrawing with `m.request` in v1 #1442

Closed iamjohnlong closed 7 years ago

iamjohnlong commented 7 years ago

Description:

In v.0... you could set background: true on m.request opt out of redraw on completion

How does one do this in v1?

barneycarroll commented 7 years ago

@iamjohnlong it's not explicitly addressed in the migration docs but in v1 m.request is totally decoupled from the view — in other words all requests are 'background' now :)

Should keep this ticket open until we get that documented…

tivac commented 7 years ago

@barneycarroll that's simply not true.

https://github.com/lhorie/mithril.js/blob/713c25c9c0490a7f50368209148737bf76647dd7/index.js#L7

calls into

https://github.com/lhorie/mithril.js/blob/d1c2a44de8db3ae74c8df972c6f10cd086acb8be/request/request.js#L10

which is then executed on every call to m.request().

It won't redraw until the promise chain completes, which is useful, but there's not explicit way to say "don't redraw when this request completes".

lhorie commented 7 years ago

Ok so from the discussion in gitter, the actionables that came out were:

1 - make request("mithril") and request("mithril/request") use different request/request instances 2 - add background: true back to m.request

Though since 1) covers the case of avoiding post-request redraws, do we really need 2)?

tivac commented 7 years ago

I think the background: true approach would be way easier to reckon with when writing application code. Having to remember to require an entirely new module to make requests w/o a redraw doesn't feel very intuitive to me.

barneycarroll commented 7 years ago

Hang on guys, background: true could be the worst outcome here. The significance of 'background' at the time I asked for it was that it shouldn't block rendering altogether during its lifecycle. What about aiming for superficial parity with the event API checking for options.redraw === false before invoking the callback?

pdfernhout commented 7 years ago

Would there be any way to make cancelling a redraw in m.request be similar to event handlers (e.g. "e.redraw = false") to make Mithril easier to learn?

Personally, since libraries like RxJS support all kinds of fancy ways to combine observables (or promises made into observables), and observables can include network requests, I wonder how much m.request will get used in the way it was intended however it is implemented?

tivac commented 7 years ago

redraw: false would be fine and potentially much clearer to those without 0.2.x context, so :+1: from me.

orbitbot commented 7 years ago

I would appreciate an effort of maintaining parity between required modules and bundled ones (partial or the full mithril.js) if at all possible (I guess there might be cases where that might not be meaningful), or at the very least (very) clearly highlighting the differences in functionality between the two in appropriate documentation.

For newcomers to mithril, js frameworks or programming in general it seems quite unreasonable to have different APIs depending on if f.e. code is written in a fiddle or in a bundling environment. Unknowing users wouldn't be able to follow code examples and would need to learn somewhat arbitrarily distinct APIs. I'd say it would be better to avoid leaving footguns around if possible.

There lies the way of H̸̡̪̯ͨ͊̽̅̾̎Ȩ̬̩̾͛ͪ̈́̀́͘C̷̙̲̝͖ͭ̏ͥͮ͟Oͮ͏̮̪̝͍M̲̖͊̒ͪͩͬ̚̚͜Ȇ̴̟̟͙̞ͩ͌͝S̨̥̫͎̭ͯ̿̔̀ͅ

lhorie commented 7 years ago

Anyone else bothered by having the option name explicitly be about the render module in the request module?

tivac commented 7 years ago

I'm not.

I'm also still of the opinion that all the splitting apart of mithril isn't all that valuable. In some cases I find it actually makes determining what mithril is doing way harder. So I guess maybe my opinion isn't that surprising.

barneycarroll commented 7 years ago

@lhorie how about we break the current coupling hook completely, and rewrite the render binding in index JS to take explicit account of the redraw option? That way require stays truly generic and we're looking at an integration wrapper.

barneycarroll commented 7 years ago

@tivac I'm with you but moreover I think it's a fool's errand to try and make the completionCallback API integration generic when it clearly exists specifically for the purposes of redraw.

If we were to rip that part of the require module out, the index code would look like this:

m.request = function(){
  var request = requestService.request.apply(this, arguments)
  var options = arguments[1] || arguments[0]

  if(options.redraw !== false) request.then(redrawService.redraw)

  return request
}

This might be naive, but AFAIK the redraw callback is always deferred (macro-task?) async so any author-land promises deriving from the request would execute as expected before the next render. Does that sound right?

iamjohnlong commented 7 years ago

why not just rip out all rendering logic from request? You would have to call redraw manually but it allows more user control.

tivac commented 7 years ago

@iamjohnlong I'd be ok with that, but it does complicate usage for what is a pretty common use-case.

  1. Request Data
  2. Process Data
  3. Re-render w/ updated data

Keeping onboarding smooth is important, but so is giving people enough control. It's a balancing act 😕

leeoniya commented 7 years ago

what domvm 1.x does to keep it decoupled is provide a callback interface for step 2. (i know....so 2014)

internally the ajax wrapper will check if the result of the callback is false, would prevent redraw and return a chainable promise.

you cant really do this with a normal promise chain (as opposed to callbacks) without explicitly chaining your own redaw calls. it's kind of a middle ground.

w.fetch("/data", ..., data => { processData(data); return shouldRedraw; }).then(..)

i guess the other peice of the story is that shouldRedraw is actually invokeObservers that are bootstrapped by the user (and presumably can do redraw in them). so there's no direct knowledge of the ajax wrapper to the view layer.

pdfernhout commented 7 years ago

Some thoughts on the suggestion by @iamjohnlong on removing automatic redrawing from m.request:

There are several different situations where redrawing may need to happen in a Mithril application. These include:

I feel automatic redrawing for user-initiated UI events is brilliant and magical. It is the right default because that need to redraw after a user-initiated UI event is so very common. Performance optimization to reduce redrawing can come later if needed. So, that is all worth learning and using.

By contrast, I have mixed feelings on redrawing by default in m.request (as well as all the other cases). Why?

I'm not sure if automatic redrawing in that case is really a net benefit compared to using manual redraws. There are usually few such calls (relative to UI event handling) and they may involve a lot of complexity including error handling or cancelling the request. Just putting in manual redraws and debugging the occasional omissions in network calls seems straightforward. Using functions like m.request that provide automatic redrawing by contrast involves a tradeoff of the learning curve for using yet another function to do common operations, learning how to work around that function sometimes to avoid redraws, and then more complex library code to debug through.

I also struggled with the design idea mentioned in Mithril documentation for the original m.request. It seemed to me to imply that somehow the UI should not render until the request is done. There were warnings about needing placeholder values to avoid null reference errors if you want it otherwise. I saw delaying a redraw when making a network request as a problematical idea because you usually want to put up a UI right away and warn the user that network request is in progress -- and then redraw again later when the request was done. I also found the m.request documentation confusing because I did not know if using m.request would block all Mithril redraws until the request was done -- which was one reason I avoided using m.request.

I also was converting an application that already had an infrastructure in place for doing network requests when I learned Mithril. Thus, I would have had an easier time learning and using Mithril if there was no m.request and just a simple example of how to do manual redraws for network requests. Others might have different experiences, so I am not saying my experience was typical. But it still seems likely to me that any JavaScript developer trying Mithril will already have some way they prefer of doing network requests.

If m.request makes sense for convenience (or even just as training wheels), maybe there should also be an m.timeout function that automatically redraws for the same reason? But then m.timeout will have the same issue. People will start using m.timeout for convenience and go up some learning curve for it (e.g. how do you cancel or debug such timeouts?), and then sometimes they will not want to redraw in special cases, and then they will ask this same sort of question again. And will such m.timeout code be more confusing than if people just did standard timeouts they already know how to do and added manual Mithril redraw calls to them?

Since only two of the situations in the list above have automatic redrawing support, a Mithril developer needs to learn how to do manual redraws sooner or later. Maybe sooner is better? On the other hand, m.request is obviously very convenient even if you know how and when to do manual redraws. So, it comes down to expectations and aesthetics about consistency, developer experience, frequency of different needs, and so on as cost/benefit.

Overall it seems to me that if there is an m.request function, then it should redraw by default. Otherwise, what is the point of it in the Mithril library versus using fetch or XMLHttpRequest? But ideally the default redraw should be either easily cancellable or entirely preventable via initial configuration to support performance optimization. If m.request does not support cancelling redrawing, performance optimization can be done by avoiding m.request and using fetch or XMLHttpRequest instead.

barneycarroll commented 7 years ago

Here's my pull request for the proposal I outlined earlier (no documentation, no tests) for technical review: #1444 - the coupling is handled at the index module level.

Passing redraw: false to the options cancels the automatic redraw on resolution, similarly to how mutating events to redraw: false cancels automatic redraw on event handler stack resolution.

dead-claudia commented 7 years ago

For what it's worth, I don't see the issue with m.request(...).then(doSomething).then(m.redraw).

pdfernhout commented 7 years ago

Another possibility is that redraw configuration could involve specifying a function. The default function would wrap UI events or network events to redraw when done. But you could replace it with some other function that behaved differently or set it to null. The default for this function could be set on "m" itself to control the entire application's redraw behavior. Redrawing could still be overrideable for each UI event handler or request as a redraw argument which would be passed to that function. For an example of a similar approach done for Maquette UI events, see: https://github.com/AFASSoftware/maquette/pull/37

lhorie commented 7 years ago

I've added back the background: true option

barneycarroll commented 7 years ago

To whom it may concern, I wrote a plugin to wrap mount and redraw such that mounted 'islands' only redraw when explicitly told to — in other words, an opt out of automatic global redraws. Check it out: Mithril Islands