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

Helpers maintain reactivity after region change in before hook. #847

Open
tmeasday opened this issue Sep 22, 2014 · 14 comments
Open

Helpers maintain reactivity after region change in before hook. #847

tmeasday opened this issue Sep 22, 2014 · 14 comments

Comments

@tmeasday
Copy link
Contributor

Am I missing something here? I'm surprised this hasn't come up more often:

Repro https:/tmeasday/IR-logout :

  1. Create an account.
  2. Hit logout.
  3. Notice that the helper on the main template runs once more, leading to the error. In general this'll happen if you do Meteor.user().something in a helper.

Occurs in both IR 0.9.3 and 1.0.0-pre2

@tmeasday tmeasday added the bug label Sep 22, 2014
@tmeasday tmeasday added this to the Next Release milestone Sep 22, 2014
@tmeasday
Copy link
Contributor Author

Ok, the repro is even simpler and can work with a simple session variable too.

@cmather are we now delaying a flush before rendering or something?

@cmather
Copy link
Contributor

cmather commented Sep 22, 2014

Thanks Tom I'll try to take a look. Is this issue actually specifically related to auth? Or is it a deps issue with helpers running again when they shouldn't? What's the expected behavior vs actual behavior?

On Sep 21, 2014, at 11:13 PM, Tom Coleman [email protected] wrote:

Ok, the repro is even simpler and can work with a simple session variable too.

@cmather are we now delaying a flush before rendering or something?


Reply to this email directly or view it on GitHub.

@tmeasday
Copy link
Contributor Author

Hey @cmather - take a look at the second commit in the project, it's a very simple reproduction w/ just a Session var.

It's a deps issue with helpers running when they shouldn't. In short:

  • If I have an onBeforeAction that pause()s (or equivalent) given condition A:
  • I expect that my action + thus helpers will never run when condition A becomes true.

In my two examples, A is either "being logged out" or "a Session var is false".

@cmather cmather changed the title Helpers run after logout, even with a auth guard. Helpers maintain reactivity after region change Sep 23, 2014
@cmather cmather changed the title Helpers maintain reactivity after region change Helpers maintain reactivity after region change in before hook. Sep 23, 2014
@tmeasday
Copy link
Contributor Author

@cmather so I debugged and thought about it a little more. It's actually a bug with I:DT I guess. I can make a simple reproduction / issue against that if you'd like to simplify it.

Basically the picture looks like this:

  1. Suppose your rendered template depends on some reactive data source S(). In I:R, this happens when our onBeforeAction depends on S(). The controller's "action computation" is the active comp here.
  2. Additionally, suppose the current rendered template A has a helper that depends on S().

Now, when S() changes, triggering a template change of A -> B, the following happens:

  1. The action computation invalidates.
  2. The helper computation invalidates.
  3. The action comp re-runs, setting dynamicTemplate.template(Template.B).
  4. The dynamic template's view computation invalidates.
  5. The helper reruns [THIS IS WHERE THINGS ARE BAD].
  6. The DT view comp re-runs, destroying A, and rendering B

Does that make sense? I can definitely write it down in code if it'll be easier to understand :)

@cmather
Copy link
Contributor

cmather commented Sep 23, 2014

Thanks for this detailed breakdown tom. I wonder is there a way to manually stop reactivity on the rendered template if we know we're going to replace it anyway. So when the template value computation invalidates we call stop or something on the underlying template view but not the enclosing region view. Hmm

On Sep 23, 2014, at 12:24 AM, Tom Coleman [email protected] wrote:

@cmather so I debugged and thought about it a little more. It's actually a bug with I:DT I guess. I can make a simple reproduction / issue against that if you'd like to simplify it.

Basically the picture looks like this:

Suppose your rendered template depends on some reactive data source S(). In I:R, this happens when our onBeforeAction depends on S(). The controller's "action computation" is the active comp here.
Additionally, suppose the current rendered template A has a helper that depends on S().
Now, when S() changes, triggering a template change of A -> B, the following happens:

The action computation invalidates.
The helper computation invalidates.
The action comp re-runs, setting dynamicTemplate.template(Template.B).
The dynamic template's view computation invalidates.
The helper reruns [THIS IS WHERE THINGS ARE BAD].
The DT view comp re-runs, destroying A, and rendering B
Does that make sense? I can definitely write it down in code if it'll be easier to understand :)


Reply to this email directly or view it on GitHub.

@tmeasday
Copy link
Contributor Author

Something like?:

DynamicTemplate.prototype.template = function (value) {
  if (arguments.length === 1 && value !== this._template) {
    this._template = value;
    this._templateDep.changed();

    // we know that we are going to destroy the currently rendered template, other computations
    // that depend on the same reactive data sources may re-run first.
    //  In order to ensure that no helpers in such comps re-run in the meantime, we destroy it now.
    //  See: https:/EventedMind/iron-router/issues/847#issuecomment-56532625
    this.view.stop();

@tmeasday
Copy link
Contributor Author

tmeasday commented Oct 2, 2014

@cmather do you want me to do this?

@tmeasday
Copy link
Contributor Author

@cmather - I can't remember where we got to with this?

@cmather
Copy link
Contributor

cmather commented Oct 26, 2014

I don't think we can currently solve this problem with Blaze. We need a way to stop computations on a view somehow. And I'm not sure how to do it without destroying the view each time.

@tmeasday
Copy link
Contributor Author

@cmather - and why can we not do that? (i.e. call view._destroyView() ? ). We know we are about to tear the view down.

@cmather
Copy link
Contributor

cmather commented Oct 26, 2014

Hmm maybe that would work. Want to give it a try? I need to work through the details though. Not completely sure that will work.

@tmeasday
Copy link
Contributor Author

I think maybe we leave this one till post 1.0 ? It seems dangerous and not a big enough problem to give ourselves unnecessary headaches right now.

@cmather
Copy link
Contributor

cmather commented Oct 27, 2014

Yep

On Oct 26, 2014, at 6:07 PM, Tom Coleman [email protected] wrote:

I think maybe we leave this one till post 1.0 ? It seems dangerous and not a big enough problem to give ourselves unnecessary headaches right now.


Reply to this email directly or view it on GitHub.

@gvc
Copy link

gvc commented Jan 31, 2016

Hey guys, is this still in your roadmap to fix?

Thanks!

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

No branches or pull requests

3 participants