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

New rendered behaviour in Blaze is harmful #2001

Closed
artpolikarpov opened this issue Apr 2, 2014 · 30 comments
Closed

New rendered behaviour in Blaze is harmful #2001

artpolikarpov opened this issue Apr 2, 2014 · 30 comments

Comments

@artpolikarpov
Copy link

It’s great that instances are never re-rendered in the new rendering model. But it’s really harmfull that it only fires once per template instance.

The example of how to port to the new rendered semantics is really poor and irrelevant to the real life. I don’t want to put every variable to a separate instance, or blowup my code with helpers, or collect all template data sources in Deps.autorun just to know that a template is updated. It really blocks me from upgrade!

Why don’t provide some callback for it, updated for example?

Template.myTemplate.updated = function ( ) { ... }
Provide a callback when an instance of a template is updated.

Mmm?

@awatson1978
Copy link
Contributor

+1
I've been thinking along these lines myself. There was design value in being able to rerender the entire template when any one particular handlebar field got updated. That design goal conflicted with another design goals of the template needing to stay the same when any one particular handlebar field got updated. The takeaway? The rendered() event is overloaded, and being used for both rendered() and updated().

I very much like the idea of an updated() callback. It would be an excellent addition to the API.

@mcbain
Copy link

mcbain commented Apr 2, 2014

If you need to know when some reactive data changed use can also use a
Deps.autorun() and listen to the "real" updates instead of re-renderings of
them

2014-04-02 17:34 GMT+02:00 Artem Polikarpov notifications@github.com:

It's great that instances are never re-rendered in the new rendering
model. But it's really harmfull that it only fires once per template
instance.

The example of how to porthttps://github.com/avital/meteor-ui-new-rendered-callbackto the new
rendered semantics is really poor and irrelevant to the real life. I
don't want to put every variable to a separate instance, or blowup my code
with helpers just to know that a template is updated. It really blocks me
from upgrade!

Why don't provide some callback for it, updated for example?

Template.myTemplate.updated = function ( ) { ... }
Provide a callback when an instance of a template is updated.

Mmm?

Reply to this email directly or view it on GitHubhttps://github.com//issues/2001
.

@awatson1978
Copy link
Contributor

True, although maybe a person's design involves wiring up a timer to a template, ala the SVG clock demo, and doesn't involve any server-client publications and subscriptions. observeChanges doesn't help if you're just returning a new date value every second. Or, better yet, returning a new date value on window resize.

@mcbain
Copy link

mcbain commented Apr 2, 2014

My post was about a Deps.autorun in the template :-)

2014-04-02 17:49 GMT+02:00 Abigail Watson notifications@github.com:

True, although maybe a person's design involves wiring up a timer to a
template, ala the SVG clock demo, and doesn't involve any server-client
publications and subscriptions. observeChanges doesn't help if you're just
returning a new date value every second. Or, better yet, returning a new
date value on window resize.

Reply to this email directly or view it on GitHubhttps://github.com//issues/2001#issuecomment-39347117
.

@awatson1978
Copy link
Contributor

How would that work? Can you provide a pattern or example?

Here's a use case:

<template name="homePage">
 <div id="homePage">
   <h2>{{title}}</h2>
   {{resized}}
  </div>
</template>
Session.setDefault('resize', null);
Meteor.startup(function () {
  $(window).resize(function(evt) {
    Session.set("resize", new Date());
  });
});

Template.homePage.resized = function(){
  return Session.get('resize');
}

Session.setDefault('pageTitle', "Home Page");
Template.homePage.title = function(){
  return Session.get('pageTitle');
}

The goal is to fire something when both the title is changed, and when the page is resized. Like so:

Template.homePage.updated() = function(){
  console.log('template updated at: ' + new Date());
}

How would we do that with Deps.autorun? I don't understand how that could be done.

@mcbain
Copy link

mcbain commented Apr 2, 2014

Put Deps.autorun in rendered()
Within the passed in function invoke e.g. self.get('title')

On any data change these function get called again. Maybe due change of the
title from a mongo update.

Am Mittwoch, 2. April 2014 schrieb Abigail Watson :

How would that work? Can you provide a pattern or example?

Here's a use case:

{{title}}

{{resized}}

Session.setDefault('resize', null);Meteor.startup(function () {
$(window).resize(function(evt) {
Session.set("resize", new Date());
});});
Template.homePage.resized = function(){
return Session.get('resize');}
Session.setDefault('pageTitle', "Home Page");Template.homePage.title = function(){
return Session.get('pageTitle');}

The goal is to fire something when both the title is changed, and when the
page is resized. Like so:

Template.homePage.updated() = function(){
console.log('template updated at: ' + new Date());}

How would we do that with Deps.autorun? I don't understand how that could
be done.

Reply to this email directly or view it on GitHubhttps://github.com//issues/2001#issuecomment-39349207
.

@awatson1978
Copy link
Contributor

Hmmm... so you're suggesting something like this?

Template.homePage.rendered = function(){
  Deps.autorun(function(){
     var titleText = Session.get('pageTitle');
     var resizeDate= Session.get('resize');
     console.log('Page title was ' + titleText + 'when resized at ' + resizeDate);
  });
}

Well, that's not too terrible. I feel like it might be okay for my purposes, since I tend to use reactive context fairly frequently. It would break down with something like this though:

Template.homePage.title = function(){
  return Math.random();
}

Hmmm...

@mquandalle
Copy link
Contributor

If you put a Deps.autorun inside a rendered callback you'll need to stop id on destroyed:

var handler;
Template.homePage.rendered = function(){
  handler = Deps.autorun(function(){
    var titleText = Session.get('pageTitle');
    var resizeDate= Session.get('resize');
    console.log('Page title was ' + titleText + 'when resized at ' + resizeDate);
  });
};

Template.homePage.destroyed = function() {
  handler && handler.stop();
};

Otherwise you'll have memory leaks.

@mizzao
Copy link
Contributor

mizzao commented Apr 2, 2014

I've been trying to port a lot of things to the new rendered semantics (sharejs, autocomplete, tutorials) yet it seems in many ways the only way to go is to write my own UI.Component. However, how this is used is not well documented and the usages of UI.Component are haphazardly sprinkled around the Meteor codebase with arbitrary _.extend calls to define its behavior, making it impossible to figure it out from source.

Putting a Deps.autorun inside rendered is no match for the old behavior because it assumes you have access to whatever reactive variables are used to render the component, instead of just using the data context; this makes it impossible to write smart package UI components (see all three examples above) because there is no encapsulation.

Moreover, the porting to helpers from rendered makes a lot of things impossible because helpers don't have access to the template instance.

</rant>

@mquandalle
Copy link
Contributor

And the component API is going to considerably change in the coming weeks/months...

@mizzao
Copy link
Contributor

mizzao commented Apr 2, 2014

I've spent a couple of days trying to figure out how to write some existing packages under the new semantics, and it's been incredibly difficult without any documentation for UI.Component. Horrible experience...

To summarize, the two approaches that have been proposed are both problematic because

  • In the Deps.autorun approach you need to have access to whatever reactive variable generates the change that you want to do some operation on. That is all fine and dandy when you are writing the app with your own Session variables, but does not work when you are writing a package.
  • In the helper approach, you don't have access to the template instance and cannot make use of the stored state of the template.

@davidworkman9
Copy link
Contributor

If you do put Deps.autorun inside rendered and that template is rendered multiple times(example: {{#each}} {{> item}} {{/each}}) you have to keep track of multiple handlers, it gets ugly fast. In one case my code went from 7 lines in a rendered function to 27, and I still haven't got it working quite right yet.

@awatson1978
Copy link
Contributor

Okay, looking at this a bit more, I think there may be a much cleaner workaround, based on the resize() pattern, that doesn't require Deps.autorun.

Create a hidden div in your template, and add a field called updated (or whatever).

<template name="fooBar">
  <div id="fooBar" class="panel">
    <h2>{{ getTitle}}</h2>
    <p>{{ getText}}</p>
    <div id="carousel"></div>
  <div> 
  <div class="hidden">{{ updated }}</div>
</template>

On each field that you want to trigger a rerender, opt into the update function by setting the 'updated' session variable with the current date. Then return the 'updated' session variable to the hidden field in the template.

Session.setDefault("updated", null); 

Template.fooBar.getTitle = function(){
  Session.set("updated", new Date());
  return this.title;
};
Template.fooBar.getText = function(){
  Session.set("updated", new Date());
  return this.text;
};

Template.fooBar.updated = function(){
  // add stuff you want to happen on each rerender here
  console.log('Page title was ' + this.title + 'when updated at ' + Session.get("updated"));

  // for instance, 
  $('#carousel').owlCarousel();

  // this is where the rerendering magic happens  
  return Session.get('updated');
};

// generalize the pattern with the following
UI.registerHelper('updated', function(){
  return Session.get('updated');
});

@swese44
Copy link

swese44 commented Apr 2, 2014

+1

I haven't seen any feasible workarounds without Deps.autorun() boilerplate or some hacky techniques, we need an updated callback as part of a standard API. I appreciate the work to increase performance, but the 0.8.0 update took away a very important callback.

@dgreensp
Copy link
Contributor

dgreensp commented Apr 3, 2014

Why is it important to know when a text node changes, or when any of several different text nodes changes? I'd like to hear more about that.

@mizzao, agreed, the modularity and encapsulation story for components is terrible right now. We're working on it. We had to cut a lot to ship 0.8. In retrospect, it was premature to even implement some of the Component machinery that currently operates under the hood, since a lot of it wasn't right. There's a short list of what people really need to write components, like local reactive state. Let's not get too into that on this thread.

That said, I'm curious how it helps component authors to know when part of a template is being updated. Presumably a component does have reactive access to its inputs.

I suspect there's a Deps-level solution to whatever the use case is here, i.e. a solution at the level of the view-model rather than bouncing the reactivity off of the template.

@dgreensp dgreensp added feature and removed feature labels Apr 3, 2014
@mizzao
Copy link
Contributor

mizzao commented Apr 3, 2014

Hi @dgreensp, I don't completely understand your last sentence, but one of the things that's very frustrating with custom components right now is the inability to embox fields of the data context. Usually these are passed in from reactive variables, but right now they are just accessed as constants and there's no way to UI.emboxValue that input. What seems really necessary to write components is some way to get at each of the context values reactively so that they can be used to take certain actions when inputs change. This is highly necessary given how a template is rendered and updated.

I don't understand why

Presumably a component does have reactive access to its inputs.

unless I'm missing something. Components written as part of apps can access reactive variables in the app, but components that are part of smart packages certainly can't.

@swese44
Copy link

swese44 commented Apr 3, 2014

Thanks @dgreensp. A simple example would be handling CSS transitions. If a name or title changes I need an event hook to remove and re-add a CSS class to highlight it.

A more complex example would be using Google maps to display a list of places. Meteor renders the map the first time then forgets about it, and I can add the map markers on template.rendered(), but when the place data changes I need a way to update the map markers.

I don't know if it's necessary to know when a specific DOM node is changed, maybe an event should fire when a template's data changes.

@mizzao
Copy link
Contributor

mizzao commented Apr 3, 2014

@swese44 I don't think your Google maps example needs the rendered behavior. It seems the 'right' way to do that would be to set up a Collection#observeChanges in the first rendered callback which would add and remove markers until the destroyed callback, which would stop the observer. Most other operations can be done on the DOM directly.

I'm just asking for a better way of telling when inputs have changed, because those are usually the ones that we can't easily listen to right now.

@awatson1978
Copy link
Contributor

  • Page styling/presentation updates.
  • Resizing page elements.
  • Kinetic typography.
  • Programatic control of the mouse cursor.
  • Gesture based user interfaces.
  • Animations in general (particularly D3).
  • Signaling haptics and mobile devices.
  • Error logging.
  • etc.

Generally speaking, it's about a) updating CSS presentation when the underlying object model changes, or b) doing machine-to-machine communications and triggering console logging, hardware API access, etc. That's what was great about the old rendered() behavior.

It's interesting that you refer to it as the 'level of the view-model'. If by 'view-model' you mean something to update the CSS based on what's in the HTML, then yes... I totally agree.

And it's interesting that swese44 just brought up Google Maps, because I was going to mention the Parties example as being a good example of the kind of app UI that people want to develop using updated().

@davidworkman9
Copy link
Contributor

The place I ran into needing it is using jQuery x-editable. After x-editable's been rendered, it stores the text in memory. So if the element's text has changed, x-editable needs to be reinitialized, or else when you go to edit it, the previous text is inserting into x-editable's input box. This is debatable about who's problem it is (x-editable or Meteor).

X-editable worked amazing in Spark but it is nothing but hell with Blaze. Blaze's dependency tracking breaks if something other than Blaze edit's the text of an element (#1998).

@awatson1978
Copy link
Contributor

@mizzao - Maybe people have their templates hooked up to timers, or mobile device inputs, and want the templates to update based on events that have nothing to do with collections. If I put in a user interface element on my page that shows mobile device connectivity status and the current time (which I have done in multiple apps), and want to resize all my elements based on tablet orientation, and adjust theme based on time of day (other things I've done in multiple apps)... those UI rerenders have anything to do with collections, so observeChanges doesn't help.

Basically, this latest change breaks tons of functionality with regards to mobile devices and animations. In short, it leans towards a server-side understanding of MVC, and leaves client-side developers a bit in the lurch. But maybe this is paving the way for Famo.us integration. In which case, maybe it's a necessary breaking change in order to get really great animations and mobile device support.

Regardless, an updated() API call would go a long way to letting people repair that functionality.

@mizzao
Copy link
Contributor

mizzao commented Apr 3, 2014

@davidworkman9 I believe x-editable actually has the ability to update its value from the DOM element, you just have to wire that up correctly, which you didn't have to do in Spark because the entire HTML node would get re-rendered.

@awatson1978 I was just giving @swese44 one example for his use case. I'm not convinced that the things you are mentioning need to be done in an updated callback. You can actually do them from any function, as long as it can properly scope to the template instance and update elements accordingly - that's the beauty of Blaze. So I would actually be looking for something that is like UI.getEnclosingTemplateInstance(DOMNode) as being more flexible than passing everything to an updated callback.

@dgreensp
Copy link
Contributor

dgreensp commented Apr 3, 2014

Thanks @mizzao, that's pretty clear. You're right, there's no API right now where templates get access to the data context as a reactive data source. Only compiled templates do it, under the hood.

We've been planning to do something about this for a while (as part of giving templates local state), but this discussion definitely helps clarify the issues for me.

My hope is to make the code that templates compile into so transparent that it's easy to drop in and do something different, like reactively capture part of the data context and set innerText on an element if you want.

@awatson1978
Copy link
Contributor

Keep in mind that updated() is the functional inverse to the deprecated preserve() callback. In Spark, the default behavior was template-wide re-rendering to be enabled by default, and for developers to opt-out with {{#constant}} and preserve(). Now that template-wide re-rendering is disabled by default in Blaze, there needs to be way to opt-in to it. Hence, updated() is analogous to preserve(), but opts-in, rather than opts-out.

And as for API semantics, have it run UI.getEnclosingTemplateInstance() in the background, perhaps, so package developers can access those fine-grained controls if they wish. But also expose an updated() API call, because it's what many app developers will be looking for.

@mizzao
Copy link
Contributor

mizzao commented Apr 3, 2014

I'm not seeing any clear way to migrate my sharejs and tutorials packages right now, and autocomplete has been done in a really ugly way due to my inability to find the right way to work with components.

@dgreensp: If we could find some way to come up with an API for this on a development branch, I'd be happy to try and use it and give feedback as I update my packages with UI components.

@awatson1978: I had an earlier discussion with either @glasser or @avital (can't find the reference at the moment) where I pointed out that the rendered callback could lead to bad coding habits. It's the same thing with the updated callback: you are getting a function call for anytime anything changes instead of just something specific being changed. Usually, you would then filter out for the specific change you are interested in. This is both inefficient and leads to bad code. So I think we can come up with something better.

@awatson1978
Copy link
Contributor

What about something inspired along the lines of Meteor.flush()? Opting into the refresh by adding Session.set('resize', new Date()) worked really well for the resizing pattern. Maybe there's a way to combine the approaches.

First, it would be nice to assume that we didn't need to add hidden handlebar fields

<template name="fooBar">
  <div id="fooBar" class="panel">
    <h2>{{ getTitle}}</h2>
    <p>{{ getText}}</p>
    <div id="carousel"></div>
  <div> 
  <!-- <div class="hidden">{{updated}}</div> -->
  <!-- <div class="hidden">{{resized}}</div> -->
</template>

And thinking through different options....

Template.fooBar.getTitle = function(){
  // Meteor.flush();
  // Template.flush();
  // Template.flush("fooBar");
  //  this.flush("fooBar");
  //  this.update("fooBar");
  //  this.onUpdate("fooBar");
  this.onUpdate();
  return this.title;
};
Template.fooBar.getText = function(){
  // Meteor.flush();
  // Template.flush();
  // Template.flush("fooBar");
  //  this.flush("fooBar");
  //  this.update("fooBar");
  //  this.onUpdate("fooBar");
  this.onUpdate();
  return this.text;
};

Template.fooBar.onUpdate = function(){
  console.log('Page title was ' + this.title + 'when updated at ' + Session.get("updated"));
  $('#carousel').owlCarousel();
};

Would something like that work? It adds a fair bit of overhead in writing boilerplate, if your basic assumption is that you want the old-style Spark rerendering behavior. But it's not too onerous.

@davidworkman9
Copy link
Contributor

@mizzao not that I can see from the docs. By default, the text is taken from the DOM element when editable is initialized. Any edits after that fact not done by x-editable don't come through.

@mizzao would you still have concerns if the updated callback passed in what had changed?

@swese44
Copy link

swese44 commented Apr 3, 2014

@awatson1978 could your resizing be handled more efficiently with CSS media queries?

@awatson1978
Copy link
Contributor

@swese44 - CSS media queries were my starting point. ;)

The resizing pattern is for when you're wanting to get really complex resizing and layout, and use custom layout functions (ie. algebraic and trigonometric layout, laying out elements using sin, cosin, golden ratio, etc) and apply those functions to dozens or hundreds of objects on screen at once. Think D3 style page layouts.

But yes... media queries are ideal to begin with, and are an essential part of layout. They hit a wall at a certain point, though.

@avital avital changed the title New rendered behaviour in Blaze is harmfull New rendered behaviour in Blaze is harmful Apr 3, 2014
@dgreensp dgreensp removed the confirmed label Apr 4, 2014
@dgreensp
Copy link
Contributor

dgreensp commented Apr 4, 2014

I'm closing this and opening #2010. The rendered callback isn't a good way to react to a template argument changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants