veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
202 stars 29 forks source link

FlowRouter.setQueryParams() always causes full route re-render #94

Closed drone1 closed 2 years ago

drone1 commented 2 years ago

I'm having an issue:

I'm trying to use FlowRouter.setQueryParams() without causing a re-render. I'd simply like my Tracker, which watches the query string via FlowRouter.getQueryParams(), to catch query string updates. Is there a particular pattern or other recommendation for doing this?

I tried throwing my call to FlowRouter.setQueryParams() into a FlowRouter.withReplaceState() callback just for the hell of it but no luck.

Thanks!

dr-dimitru commented 2 years ago

Hello @drone1 ,

I'm trying to use FlowRouter.setQueryParams() without causing a re-render

It depends from how you render, can you post your code here?

drone1 commented 2 years ago

It will take me some time to get a simple code example to show. For now, if it's easy can you simply tell me what I would need to do not to re-render when using FlowRouter.setQueryParams(), if possible? Would appreciate it. Thanks.

On Tue, Jun 7, 2022 at 2:52 PM dr.dimitru @.***> wrote:

Hello @drone1 https://github.com/drone1 ,

I'm trying to use FlowRouter.setQueryParams() without causing a re-render

It depends from how you render, can you post your code here?

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1148626295, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPDPDOW2TLNB4FW2FRLVN5AX3ANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1 really would need to know how do you render it. Do you use this.render() or blaze-renderer package?

drone1 commented 2 years ago

Ah OK. I use this.render()!

On Tue, Jun 14, 2022 at 4:46 PM dr.dimitru @.***> wrote:

@drone1 https://github.com/drone1 really would need to know how do you render it. Do you use this.render() or blaze-renderer package?

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1155294440, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPAKKUU2CZ2TKO3WLGDVPCLLDANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1 have you updated to the lates release? Perhaps it was fixed

drone1 commented 2 years ago

Just updated and tested. Still re-renders the page. forceReRender is not set to true, and I believe it defaults to false, so i'm not sure what else to try. Thanks.

On Tue, Jun 14, 2022 at 4:51 PM dr.dimitru @.***> wrote:

@drone1 https://github.com/drone1 have you updated to the lates release? Perhaps it was fixed

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1155301463, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPFFSWTHSQ6QY7Z5H4LVPCMAJANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

We would need a reproduction repo for this one, or at least post router definition

drone1 commented 2 years ago

OK, got some code together for you.

routes.js:

import './foo.js'

// routes.js
let count = 0
let firstRender = true

FlowRouter.route('/foo', {
    forceReRender: false,

    action: function (params, query, data) {
        console.log('test')

        this.render('foo', { count })
        count++

        if (firstRender) {
            // Only do this once:
            setInterval(
                () => FlowRouter.setQueryParams({ test: count }),   // Causes re-render
                2000
            )

            firstRender = false
        }
    }
})

foo.html:

<template name="foo">
    render {{count}}
</template>

foo.js:

import './foo.html'

Many thanks.

dr-dimitru commented 2 years ago

This isn't 100%. You might be able to solve it by using layout template with {{>yield}}

drone1 commented 2 years ago

If someone could point me to the documentation for "yield" it would be greatly appreciated. I can find it explained in the FlowRouter docs, BlazeJS docs or anywhere else. I also see examples of it used with Iron:Router which is confusing. Anyone? What exactly implements "yield"? And what does it do? A link would be grealty appreciated. Thank you!

On Tue, Jun 14, 2022 at 10:01 PM Jon Lippincott @.***> wrote:

Can you be more specific please? Thank you.

On Tue 14. Jun 2022 at 20:11, dr.dimitru @.***> wrote:

This isn't 100%. You might be able to solve it by using layout template with {{>yield}}

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1155530516, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPHN4SXGMW36MVKGORLVPDDMFANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1 flow-router/templating

drone1 commented 2 years ago

Thanks, but where is the explanation for exactly what yield does? I can infer roughly but there is no crisp definition that I can see. Thanks.

On Sat 18. Jun 2022 at 07:15, dr.dimitru @.***> wrote:

@drone1 https://github.com/drone1 flow-router/templating https://github.com/veliovgroup/flow-router/blob/master/docs/templating.md

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1159364839, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPA7IAEHG2FENYKAHJLVPVLQVANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1

is the explanation for exactly what yield does?

yield helps to render as passthrough data to your template, there are some improvements into rendering process in order to optimize it and avoid re-rendering of entire page the it isn't necessary. Also it cache and reuse previously loaded templates and layouts

drone1 commented 2 years ago

Thank you so much for explaining. Okay, I have clearly been missing out on something critical here. Maybe I missed something but I feel like this could be made much more clear in the documentation? This all seems extremely useful!

I'll be sure to test and let you know how it goes.

By the way, isn't this unreachable code? At a glance it looks like it may be. https://github.com/veliovgroup/flow-router/blob/e2cbb67bf422cde79aade9ebeccbdf768f05fc90/client/renderer.js#L348

and

https://github.com/veliovgroup/flow-router/blob/e2cbb67bf422cde79aade9ebeccbdf768f05fc90/client/renderer.js#L358

(LMK if you want me to open a separate issue)

drone1 commented 2 years ago

Quick question: I'm trying to assess the amount of work this would be to refactor my application to use yield.

Is it possible to yield a child of the layout? Or does it have to be in layout (and not a child of layout)?

I've_ got a main application layout which calls Template.dynamic with another layout. And then the given page is rendered within that.

Can I use yield with this structure, or would I need to refactor? Thank you.

dr-dimitru commented 2 years ago

@drone1

this could be made much more clear in the documentation

Sure, feel free to send a PR

By the way, isn't this unreachable code?

Why? Changing object property to read it on the next/other cycle

Is it possible to yield a child of the layout?

you can create empty template, with just {{> yield}}

Or does it have to be in layout (and not a child of layout)?

Since "layout" is template, and "templates" are following HTML tree structure — everything is a "child" of layout

I've_ got a main application layout which calls Template.dynamic with another layout. And then the given page is rendered within that.

  1. Replace Template.dynamic with {{>yield}}
  2. Use .render() passing layout and template names, instead of changing/assigning variable passed to Template.dynamic

Hope that helps

drone1 commented 2 years ago

Sure, feel free to send a PR

As I have a relatively vague understanding of what yield does, I do not think I am a great person to write the documentation.

Replace Template.dynamic with {{>yield}} Use .render() passing layout and template names, instead of changing/assigning variable passed to Template.dynamic

(We're getting off-top but this is very helpful to me - thanks!): Thank you for these suggestions. I've done this, but my application is still calling "action" on routes which should be "re-used" (forceReRender: false) -- am I doing something wrong?

app renders layout renders template

Currently, after your suggestion, app renders layout using {{> yeield }}

But since most of the heavy lifting happens in 'template', do I need to refactor and merge app and layout or can I put a yield in app and in layout?

I have tried it, and it did not work. Use of yield is currently not supported in this way, yes? I am just trying to make sure.

To return to the topic at hand, I have updated the example to use yield and FlowRouter.setQueryParams() still causes a full re-render. Count increases.

foo.html:

<template name="layout">
    {{> yield}}
</template>

<template name="foo">
    render {{count}}
</template>

routes.js:

// routes.js
let count = 0
let firstRender = true

FlowRouter.route('/foo', {
    forceReRender: false,

    action: function (params, query, data) {
        console.log('test')

        this.render('layout', 'foo', { count })
        count++

        if (firstRender) {
            // Only do this once:
            setInterval(
                () => FlowRouter.setQueryParams({ test: count }),   // Causes re-render
                2000
            )

            firstRender = false
        }
    }
})

Thanks.

dr-dimitru commented 2 years ago

@drone1

  1. action method will be always called on URL change, this templating technique is only regarding how's actual .render() of a Blaze template
  2. How would you explain "full rerendering"? What is it exactly in your case? (perhaps your screen record is the fastest way to explain)
drone1 commented 2 years ago

OK, that makes sense.

Would you be so kind as to please answer the questions I asked in italics? It would help me a lot.

On Tue, Jun 21, 2022 at 4:59 PM dr.dimitru @.***> wrote:

@drone1 https://github.com/drone1

  1. action method will be always called on URL change, this templating technique is only regarding how's actual .render() of a Blaze template
  2. How would you explain "rerendering" in your case?

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1161866557, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPAP5NOSYPGXYBRDCHLVQHKFVANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1

app renders layout renders template Currently, after your suggestion, app renders layout using {{> yeield }}

not sure how this would be possible, I believe template will override the app template. In short — place {{>yield}} inside whatever template is passed as a first argument into .render() method

But since most of the heavy lifting happens in 'template', do I need to refactor and merge app and layout or can I put a yield in app and in layout?

I suggest to figure out "re-rendering issue" on empty template before making things complex

I have tried it, and it did not work. Use of yield is currently not supported in this way, yes? I am just trying to make sure.

Having more than one {{>yield}}, shouldn't work (in theory)

To provide further support I'd need to have clear understanding what do you mean by full re-rendering

drone1 commented 2 years ago

yield helps to render as passthrough data to your template, there are some improvements into rendering process in order to optimize it and avoid re-rendering of entire page the it isn't necessary. Also it cache and reuse previously loaded templates and layouts

What did you mean by avoid re-rendering of the entire page the it isn't necessary? That's what I mean by full re-rendering. :)

dr-dimitru commented 2 years ago

@drone1 I need to see visual or minimal reproduction code. Why do you think it's fully re-rendered? Do you see elements removed from DOM and placed back again?

drone1 commented 2 years ago

I was able to optimize my application by caching results of waitOn(). I will start a new issue if I find something odd, but this made a huge difference. So I'm good for now.

So, back to the topic of this issue. :)

Reproduction code for this issue is in my comment above.

The routing still increases the 'count' every 2 seconds (see setInterval), because calling FlowRouter.setQueryParam() causes the page to re-render.

Would you not expect this? There is nothing reactively getting the query parameter.

dr-dimitru commented 2 years ago

Reproduction code for this issue is in my https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1161854792.

The routing still increases the 'count' every 2 seconds (see setInterval), because calling FlowRouter.setQueryParam() causes the page to re-render.

action() hook is expected to run on every URL change

Would you not expect this? There is nothing reactively getting the query parameter.

Black box, there are no details about your template an its structure

was able to optimize my application by caching results of waitOn(). I will start a new issue if I find something odd, but this made a huge difference. So I'm good for now.

You had waitOn and never mentioned it...

drone1 commented 2 years ago

action() hook is expected to run on every URL change

So now you literally just answered the initial question: If this.render() is called within action(), FlowRouter.setQueryParams() will cause a page to re-render. Is that correct?

Black box, there are no details about your template an its structure

I don't think we are talking about the same thing here. It's not a black box -- I have included full reproduction source code for you in this thread twice.

waitOn I have in my application, not in the reproduction code I have included twice pertaining to the specific topic this issue was created for, which is FlowRouter.setQueryParams() causing pages to re-render. Perhaps you can try running the reproduction code and seeing if it's behaving as you might expect. Thanks!

dr-dimitru commented 2 years ago

So now you literally just answered the initial question: If this.render() is called within action(), FlowRouter.setQueryParams() will cause a page to re-render. Is that correct?

No, action() hook is always called on URL change

I have included twice pertaining to the specific topic this issue was created for, which is FlowRouter.setQueryParams() causing pages to re-render

Does it mean that if you call .go() method or click a link — there are no re-rendering, is that what you mean?

dr-dimitru commented 2 years ago

@drone1 Feel free to close it in case if the issue is solved on your end.

dr-dimitru commented 2 years ago

Hello @drone1, have you found a way to solve this on your end?

dr-dimitru commented 2 years ago

Closed due to silence at issue owner end. Feel free to reopen it in case if the issue still persists on your end.

drone1 commented 2 years ago

Can you be more specific please? Thank you.

On Tue 14. Jun 2022 at 20:11, dr.dimitru @.***> wrote:

This isn't 100%. You might be able to solve it by using layout template with {{>yield}}

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/94#issuecomment-1155530516, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPHN4SXGMW36MVKGORLVPDDMFANCNFSM5NURLVLQ . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 2 years ago

@drone1 templating docs are self-explanatory

dr-dimitru commented 1 year ago

@drone1 I've faced similar issue recently and realized that this statement is only true when data passed to the template not changed:

// without this option template won't be re-rendered // upon navigation between different "item" routes // e.g. when navigating from /item/1 to /item/2