veliovgroup / Meteor-flow-router-title

Change document.title on the fly within flow-router
https://atmospherejs.com/ostrio/flow-router-title
BSD 3-Clause "New" or "Revised" License
25 stars 4 forks source link

route title not reactive #6

Closed jdmswong closed 6 years ago

jdmswong commented 7 years ago

I have found that the title function is not reactive-sensitive. my test ( have subbed out the post title with "my blog" for now:

This prints undefined then my blog post name, demonstrating reactivity.

blogSection.route('/:date/:postTitle/', {
    name: 'Post',
    title: function(params){
        Tracker.autorun(()=>{
            var blog = Blog.findOne({
                name: "My Blog",
            });
            console.log("route: ",blog && blog.name); // TODO remove

        })

    },

this prints undefined, and does not set the title to the blog name. If this function were reactive it would as per the example above

blogSection.route('/:date/:postTitle/', {
    name: 'Post',
    title: function(params){
            var blog = Blog.findOne({
                name: "My Blog",
            });
            console.log("route: ",blog && blog.name); 
                        return blog.name;
    },
dr-dimitru commented 7 years ago

you have to add waitOn for subscription to be ready

jdmswong commented 7 years ago

https://github.com/VeliovGroup/flow-router#waiton-hook

dr-dimitru commented 7 years ago

@jdmswong yes, it solved this issue?

jdmswong commented 7 years ago

I didn't want the page to wait on the subscription being ready, so I displayed something else in the title. Out of curiosity are there any plans to make the title function reactive in the future?

dr-dimitru commented 7 years ago

Yes, I'll work on your issues soon, hope to put my hands on before end of this week. Ping me if I won't response for more than week, can forget, sorry... to busy right now

dr-dimitru commented 7 years ago

@jdmswong I took a look on demo, and code of this package. Reactive data-source should work.

As in this example, i18n.get() returns ReactiveVar and title is updated.

FlowRouter.route('/me/account', {
  name: 'account',
  title: function() {
    // In this example we used `ostrio:i18n` package
    return i18n.get('account.document.title'); 
  }
});

So, could you please try:

blogSection.route('/:date/:postTitle/', {
    name: 'Post',
    title: function(params){
            var blog = Blog.find({
                name: "My Blog",
            }).fetch();
            if (blog[0] && blog[0].name) {
                return blog[0].name;
            } else {
                return "Loading....";
            }
    }
});
jdmswong commented 7 years ago

I did, the title stays at "Loading..."

dr-dimitru commented 7 years ago

To make sure, type this into browser's console:

Blog.find({name: "My Blog"}).fetch()
jdmswong commented 7 years ago

It returns the blog as expected, additionally the page itself is populated with the correct data

dr-dimitru commented 7 years ago

Okay, I'll take look

dr-dimitru commented 7 years ago

Hi @jdmswong ,

Sorry for delay, but if you still having this issue could you please test it on latest release?

dr-dimitru commented 7 years ago

@jdmswong any news on your end?

dr-dimitru commented 7 years ago

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

nicooprat commented 6 years ago

It's an interesting issue because we're supposed to do component-based subscriptions, but title is one of the thing that could not easily be managed at this level. I don't want to block routing with waitOn too. Do you settled on any other solution since then? Thanks.

dr-dimitru commented 6 years ago

@nicooprat using data hook. But anyways you probably will end up using waitOn. Why you're afraid to use waitOn hook?

dr-dimitru commented 6 years ago

we're supposed to do component-based subscriptions

Got it now. Well, as soon as data will be available on the cursor, no matter where subscription is called, a title will be updated as stated here.

nicooprat commented 6 years ago

You're right, using .find().fetch()[0] is reactive unlike doing .findOne(). Didn't think it would be that simple. Sorry for issue bumping, this can be closed again ;) Thanks for your reactivity!

dr-dimitru commented 6 years ago

@nicooprat I wish you to find a solution to fit your needs. ๐Ÿ˜ƒ Ping me if you will have more questions. ๐Ÿ‘‹

nicooprat commented 6 years ago

I might have spoke too fast... Looks like it was working because I already subscribed to needed datas.

Further tests aren't successful, I reduced the issue to a simple test:

FlowRouter.route('/test', {
  name: 'test',
  title: function() {
    return Session.get('test')
  }
});

Setting new values like Session.set('test', 'foo') never fires the function again. Did I miss something obvious?

dr-dimitru commented 6 years ago

@jdmswong this might not work with Sessions. Could you create separate issue with request to support reactive Sessions?

nicooprat commented 6 years ago

Actually I tried with Session to be sure, because it's the simplest reactive thing I could think of, but doing it with cursors didn't work too. I can create a new issue but I think it's still the same problem... Here's a example of route definition:

FlowRouter.route('/:storyId', {
  name: 'story',
  title({storyId}) {
    // This is only fired once, cursors are empty
    // Subscriptions are done in templates
    // When data is available, this doesn't run again
    const story = Stories.find(storyId).fetch()[0]
    return story && story.title
  },
  action: function(params, queryParams) {
    BlazeLayout.render(...)
  }
})

I also tried const story = Stories.find(storyId).fetch() (without [0]), or even const story = Stories.find(storyId), no luck.

dr-dimitru commented 6 years ago

I can create a new issue but I think it's still the same problem... Here's a example of route definition:

Yes, that will be great. You can make it more generous - to fix reactive sources.

afrokick commented 4 years ago

It doesn't work for ReactiveVar.

import { FlowRouter } from 'meteor/ostrio:flow-router-extra';

const test = new ReactiveVar(1);

Meteor.setInterval(() => {
  test.set(test.get() + 1);
  console.log(`new:${test.get()}`);
}, 1000);

FlowRouter.route('*', {
  name: 'Test.name',
  action() {
    console.log('rendered');
  },
  title() {
    const t = test.get();

    console.log(`change title:${t}`);

    return `count ${t}`;
  },
});
dr-dimitru commented 4 years ago

Hello @afrokick ๐Ÿ‘‹ Please make sure youโ€™re on the latest meteor and package releases. Itโ€™s better to create new issue thread as this one is closed. Please follow our ISSUE_TEMPLATE when you create a new issue.