iron-meteor / iron-router

A client and server side router designed specifically for Meteor.
MIT License
1.98k stars 413 forks source link

why this.ready() called twice? #718

Closed MaximDubrovin closed 10 years ago

MaximDubrovin commented 10 years ago

Problem: all code handled to this.ready() called twice. Question: IR always invalidate older subscriptions even if it is the same sub?

Example: If go from route mysite.com/album/:some_id to mysite.com/album/:another_id then all code handled to this.ready() called twice.

Route mysite.com/album/:id doesn't have any subs and waitOn handler.

It uses data from global subscription in Router.configure.

This sub isn't parameter dependent. It just return all published albums.

Iron-Router Config

Router.configure({
    waitOn: function() {
       return Meteor.subscribe('albums_published');
    }
});

Album Route:

this.route('albumShow', {
        path: '/album/:id',
        layoutTemplate: 'layout_for_album_mode_show',
        yieldTemplates: {
            'nav_for_header': { to: 'nav' },
            'footer': { to: 'footer' }
        },
        template: 'album_mode_show',
        data: function() {

               album = Albums.findOne({ _id: this.params.idOrSlug }, { reactive: false });

            var albumId = album._id;

            var order = album.order;
            var prevOrder = order - 1;
            var nextOrder = order + 1;

            var prevAlbum = Albums.findOne({ order: prevOrder }, { reactive: false });
            var nextAlbum = Albums.findOne({ order: nextOrder }, { reactive: false });

            return {
                album: album,
                albumId: albumId,
                prevAlbum: prevAlbum,
                nextAlbum: nextAlbum
            }
     },
        action: function() {
            if (this.ready()) {
                this.render();

               //data dependent code here will be called twice
               //if I go to this route but with another :parameter
               var album, albumCoverImage, albumTitle, albumNote, block;
                album = this.data().album;

                albumCoverImage = S.utils.getFirstAlbumImage(album.photos) ? S.utils.getFirstAlbumImage(album.photos)[0] : null;
                albumTitle =  album.title ? album.title.toUpperCase() + ' • nana.photography album' : 'nana.photography album';

                if (album.note && album.note.text) {
                    albumNote = album.note.text;
                }

                SEO.set({
                    title: albumTitle,
                    meta: {
                        'description': albumNote ? albumNote : '',
                        'twitter:card': 'summary_large_image',
                        'twitter:creator': '@MaxDubrovin'
                    },
                    og: {
                        'title': albumTitle,
                        'description': albumNote ? albumNote : '',
                        'image': albumCoverImage ? albumCoverImage.url + '-/resize/1280x/-/quality/best/' : window.location.origin + '/img/favicon-256.png'
                    }
                });
            }
        }
    });
MaximDubrovin commented 10 years ago

Considerations: According to docs every route is running in a reactive computation.

If I'm on route mysite.com/album/:some_id and click link to mysite.com/album/:another_id then Iron-Router thinks that I go to another route and he is right.

According to this @cmather comment for IR going to the another route means that:

All right again.

But sub already in global config:

Router.configure({
    waitOn: function() {
       return Meteor.subscribe('albums_published');
    }
});

Am I right that it works as re-subscribe before the next flush?

MaximDubrovin commented 10 years ago

Also: If I firstly go to or do hard page refresh mysite.com/album/:some_id then any code handled tothis.ready() called once.

But: If I'm on route mysite.com/album/:some_id and click link to mysite.com/album/:another_id then any code handled tothis.ready() called twice.

MaximDubrovin commented 10 years ago

Here the app: (there was a link)

console.log('READY!') handled on this.ready()

It's hard to notice on desktop but on mobile devices which are slowly you can notice how browser tab title will be set and blink multiple times when you go from one album to another. Meta tags setting handled on this.ready() that guarantees data availability.

Please, ignore ssl warning because there is selfsigned certificate on dev server.

cmather commented 10 years ago

Hey @MaximDubrovin,

Thanks for all of this info!

In the current release and in devel this.ready() will invalidate its outer computation any time an item in the list changes its ready value. For example, if you're waiting on two subscriptions, this.ready() will invalidate the outer computation twice. It's return value will be true if all items in the list are ready, and false if at least one item in the list is not ready.

If you're noticing that an item is in the waitlist from a previous route, that could be a bug. It would be nice to have a reproduction that shows that case exactly, along with your expected behavior vs. actual behavior and some steps I can take to reproduce. If your link above is simple enough to demonstrate the actual vs expected behavior with steps I can follow I'll take a look. If it's not, it would be really helpful if you could make it that way in case this is a hidden bug :).

MaximDubrovin commented 10 years ago

Thanks, you answer gave me ideas where I need to dig.

I added you as a collaborator to my repo, @cmather

The whole app is here. Album route, for example.

(there was a link)

On the bottom of the album you can go to the next or previous album. As I wrote console.log('READY!') handled on this.ready() and it will be called twice.

Subscription the same. Just all published albums. It is in the Router.configure.

Want to clarify:

I'm on route and successfully subscribed to just one publication. No other subs on this route.

Then I go to another route with the same subscription to the same publication. No other subs on this route.

IR anyway invalidate my subscription even if it is completely the same?

A lot of thoughts, but I think It's better to clarify all step by step.

Tell me If you need more info or repo isn't reproducible because not all files are committed.

MaximDubrovin commented 10 years ago

Seems subs always invalidate when you go to new route.

It happens onRun. After WaitList has been cleared, route controller starts to construct new wait list based on your new route.

MaximDubrovin commented 10 years ago

@cmather, I found the cause. Briefly: ms-seo package subscribes for own created collection and uses Router.current() which is reactive variable.

Because SEO.set method that subscribes placed inside this.ready() it cause subscriptions invalidation. And it cause this causes that this.ready() will be called second time when data from new sub will be available.

Overall tip for such errors: don't place code that subscribes inside this.ready() or any other reactive dependency.

More info later.

MaximDubrovin commented 10 years ago

Summary:

I used SEO.set method from ms-seo package inside if (this.ready()) {} handler to set meta tags based on route data.

I faced with a problem that this.ready() called twice therefore code code inside if (this.ready()) {} handler ran twice. From visual point of view I noticed flickering in browser tab title.

Cause:

SEO.set method from ms-seo package uses Router.current() which is reactive variable

On this reactive variable [depends](SEO.set method from ms-seo package) ms-seo subscription.

Because ms-seo subscription take place inside if (this.ready()) {} handler it caused this.ready() invalidation.

Overall tip for such errors: don't place code that subscribes inside this.ready() or any other reactive dependency.

cmather commented 10 years ago

Glad to we're able to track this down. I'm hoping this will be cleaned up and easier to deal with in the next major release. You shouldn't have to think about this stuff too much. You should be able to use this.ready() when you want without worrying about it causing problems.

On Jul 26, 2014, at 3:40 AM, MaximDubrovin notifications@github.com wrote:

Summary:

I used SEO.set method from ms-seo package inside if (this.ready()) {} handler to set meta tags based on route data.

I faced with a problem that this.ready() called twice therefore code code inside if (this.ready()) {} handler ran twice. From visual point of view I noticed flickering in browser tab title.

Cause:

SEO.set method from ms-seo package uses Router.current() which is reactive variable

On this reactive variable depends ms-seo subscription.

Because ms-seo subscription take place inside if (this.ready()) {} handler it caused this.ready() invalidation.

Overall tip for such errors: don't place code that subscribes inside this.ready() or any other reactive dependency.

— Reply to this email directly or view it on GitHub.