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

Publication indicates it is ready before all data is published #67

Closed
sahanDissanayake opened this issue Dec 5, 2015 · 36 comments · Fixed by #179
Closed

Publication indicates it is ready before all data is published #67

sahanDissanayake opened this issue Dec 5, 2015 · 36 comments · Fixed by #179
Labels

Comments

@sahanDissanayake
Copy link

Hi @reywood ,

Is there a way to debug why I'm not getting the joined data from the server ?

This is just returning the results

Meteor.publishComposite('jobs', {
    find() {
        return Jobs.find();
    },
    children: [
        {
            find(job) {
                return Properties.find(
                    {
                        _id: job.propertyID
                    },
                    {
                        sort:{createdAt: -1}
                    }
                );
            }
        },
        {
            find(job) {
                return Meteor.users.find(
                    {_id: job.createdBy},
                    {limit: 1}
                );
            }
        }
    ]
});

As this one

Meteor.publish('jobs', {
  return Jobs.find();
});
@sahanDissanayake
Copy link
Author

I;m using react on my project too something similar to this https://github.com/micouz/meteor-react-tutorial-next

@mquandalle
Copy link

This may be caused by publishComposite returning the ready DDP message before the remaining data. I have a similar problem while trying to subscribe on the server with FastRender wekan/wekan#431.

@sahanDissanayake
Copy link
Author

Yep.. This is an issue.. The ready event comes through before the data is fetched.. Any thoughts on a fix @mquandalle ?
@reywood ??

getMeteorData() {

        const propertyHandle = Meteor.subscribe('properties');

        const data = {};
        console.log(handle.ready())
        if( propertyHandle.ready()) {

            data.properties = Properties.find().fetch();
        }

        data.currentUser = Meteor.user()

        return data;
    }

Here the propertyHandle.ready() output true even if the subscription is not ready to send the data down.. Because of this I cannot use FastRender, and React SSR too..

@sahanDissanayake
Copy link
Author

@reywood here we go, https://github.com/sahanDissanayake/hello-react-meteor

You could see the difference when you change publishComposite to be normal meteor publish

@sahanDissanayake
Copy link
Author

Any thoughts on the issue ??

@mquandalle
Copy link

I tried to fix this issue, but it may require a rather large refactoring of the internal implementation. I would love to read @reywood on the matter.

We should take inspiration of Meteor.publish implementation here. Especially it seems that under the hood Meteor use observeChanges instead of `observe to publish the cursor.

@sahanDissanayake
Copy link
Author

@mquandalle @reywood Should I open a new issue for the handle.ready() functionality issue ??

@TedAvery
Copy link

+1

@eXon
Copy link

eXon commented Jan 3, 2016

This fix is badly needed or SSR will be impossible with this package. Is there a way I can help?

@sahanDissanayake
Copy link
Author

@reywood Do you think there is a fix for this ? Like @eXon mentioned can we help somehow to get this fixed ?

This is a comment found regarding the issue

So yeah, regarding reywood:publish-composite, #67 is an issue for SSR since it currently marks the subscription as ready as soon as the data for the "primary" cursor is sent, and data for the "secondary/joined" cursors get sent after that.

I'm not sure whether it's the only issue though : trying to use the latest meteor-react-router-ssr with reywood:publish-composite, I'm seeing no data at all in the server-side subscription, even on the primary cursor

@TedAvery
Copy link

I'm still fairly new to Meteor, but as a temporary workaround, I believe I achieved what this package helps with by manually chaining together two subscriptions in getMeteorData of my React component.

FormContainer = React.createClass({
  mixins: [ReactMeteorData],
  getMeteorData() {
    var data = {};
    var shortname = this.props.shortname;
    var handle = Meteor.subscribe('form_by_shortname', shortname);
    if (handle.ready()) {
      data.form = Forms.findOne({shortname: shortname});
      var handle2 = Meteor.subscribe('template_by_id', data.form.template);
      if (handle2.ready()) {
        data.form.template = FormTemplates.findOne({ _id: data.form.template });
      }
    }
    return data;
  },
  render() { ... }
}

@sahanDissanayake
Copy link
Author

@TedAvery this is good for simple structures,

But when it comes to this https://docs.mongodb.org/manual/core/data-model-design/#normalized-data-models

We need the publishComposite to work as it is suppose to

@yched
Copy link

yched commented Jan 11, 2016

@TedAvery the issue with doing that on the client is that it requires more data roundtrips. That's what this package is supposed to avoid :-)

@davegariepy
Copy link

+1

@john-osullivan
Copy link

++1

@tcastelli
Copy link

+1

@pcorey
Copy link

pcorey commented Mar 14, 2016

+1 Even from a non-SSR perspective, it would be nice (and expected) if a publishComposite didn't respond ready until all children queries are ready.

@lukehollis
Copy link

+1

@MaxTwentythree
Copy link

+1 Any news on this?

@stolinski
Copy link

+1

2 similar comments
@achapela
Copy link

achapela commented Sep 8, 2016

+1

@rodcope1
Copy link

+1

rodcope1 added a commit to rodcope1/meteor-publish-composite that referenced this issue Sep 15, 2016
@JWDobken
Copy link

JWDobken commented Feb 9, 2017

+1

2 similar comments
@ghost
Copy link

ghost commented Mar 6, 2017

+1

@mynameiskyleok
Copy link

+1

@reywood reywood changed the title Meteor.publishComposite still returns the Meteor.publish data Publication indicates it is ready before all data is published May 2, 2017
@reywood reywood changed the title Publication indicates it is ready before all data is published Publication indicates it is ready before all data is published May 2, 2017
@reywood
Copy link
Collaborator

reywood commented May 2, 2017

I've tried repeatedly to reproduce this problem, but I can't get it to happen. If anyone can create a GitHub repo that reproduces this problem, I would be very grateful.

@KoenLav
Copy link
Contributor

KoenLav commented Mar 31, 2018

We have reproduced this problem with a normal Meteor.publish as well; when publishing the relations of one of our Collections we return an array of cursors in a normal Meteor.publish and when subscribing to this publication subscription.ready() will fire before all data from the relations is actually there.

@KoenLav
Copy link
Contributor

KoenLav commented Mar 31, 2018

Also, to add: for us this problem only appears when a previously rendered component also subscribes on a subset of one (or more) of the relations.

For instance: when displaying a paginated list of a collection we subscribed on the 'listWithRelations' composite publication which provided the first x (20) results of the collection together with all the related fields (but only those items which are actually necessary to render the current list, not the entire collection).

Afterwards when we go to the create page we subscribe on the 'relations' publication (normal Meteor publication) which is supposed to load data for all the relations, but (I assume) because we were previously subscribed to only a subset of the same collection we are now subscribing to the subscription returns ready too early (because we were previously subscribed, but to a subset).

We can confirm that the create component ONLY and EXACTLY renders exactly those items which were in the 'subset subscription' and that the problem is resolved when the page is refreshed.

So it appears that Meteor does not properly destroy composite subscriptions on the client side when a component is unmounted and still believes the subscriptions to be available.

I hope this sheds some light for some other people.

The solution we have implemented for now is no longer subscribing on the subset of the relations on our list page, but simply subscribing on entire 'relations' publication right away. While not ideal, it is acceptable for our use-case.

@fabyeah
Copy link

fabyeah commented Apr 1, 2018

+1

@sakulstra
Copy link

sakulstra commented Apr 9, 2018

@KoenLav could you create a reproduction or even better a failing test? I tried by following your description but didn't succeed... as I get it this should do the trick with the testing setup provided in this package - but it doesn't as the test succeeds:

/* test/server.js */
// subset
publishComposite('limitPosts', {
    find() {
        return Posts.find({}, { limit: 1 });
    },
    children: postPublicationChildren
});
// all related
Meteor.publish('allComments', () => {
    return Comments.find();
});

/* test/client.js */
it('should throw', onComplete => {
        // subscribe to subset
        const handle1 = Meteor.subscribe('limitPosts', () => {
            expect(Posts.find().count()).to.equal(1);
            // handle1.stop(); // not sure if you want to stop the subsciption here, but it doesn't change anything
            const handle2 = Meteor.subscribe('allComments', () => {
                // everything seems to be ready now
                expect(Comments.find().count()).to.equal(5);
                onComplete();
            });
        });
    });

The above test case should describe your scenario if i'm not mistaken (the handle.stop() would emulate the unmount, but even without stopping it works as expected)

@KoenLav
Copy link
Contributor

KoenLav commented Apr 12, 2018

@sakulstra we found out that the issues was caused by us modifying the selector used in the publish functions rather than obtaining a copy of the object and modifying that. Because of this the relations of an Entity were filtered by the id's of the Entity on the previous page.

In short: our issue was not caused by publishComposite.

@cormip
Copy link

cormip commented May 18, 2018

+1

@rclai
Copy link
Contributor

rclai commented Feb 8, 2019

@KoenLav my PR #121 fixes the issue you are having.

@milanzigmond
Copy link

Has this issue been resolved? I'm having the same problem.

@redabourial
Copy link
Contributor

redabourial commented Feb 27, 2024

I have this code :

publishComposite('users', async function getUsers(realm, token) {
    const [_, company] = await decodeToken(realm, token);
    return {
        find() {
            return UsersCollection.find({
                company: company._id,
                deletedAt: null,
            }, {
                fields: { keycloakUserId: 0, company: 0 },
            });
        },
        children: [
            {
                collectionName: 'usersAttributes',
                find(user) {
                    return UsersAttributesCollection.find({
                        company: company._id,
                        deletedAt: null,
                        user: user._id,
                    }, {
                        fields: { company: 0, createdBy: 0, createdAt: 0 },
                    })
                },
                children: [
                    {
                        collectionName: 'attributes',
                        find(userAttribute, _) {
                            return AttributesCollection.find({
                                company: company._id,
                                deletedAt: null,
                                _id: userAttribute.attribute,
                            }, {
                                fields: { company: 0, createdBy: 0, createdAt: 0 },
                            })
                        },
                        children: [
                            {
                                collectionName: 'attributesChoices',
                                find(attribute, userAttribute, _1) {
                                    let ids = []
                                    if(attribute.type==="select"){
                                        ids.push(userAttribute.value)
                                    }else if(attribute.type==="multiselect"){
                                        ids.push(...userAttribute.value)
                                    }
                                    return AttributesChoicesCollection.find({
                                        _id: { $in: ids },
                                        company: company._id,
                                        deletedAt: null,
                                        attribute: attribute._id,
                                    }, {
                                        fields: { company: 0, createdBy: 0, createdAt: 0 },
                                    })
                                }
                            }
                        ]
                    }
                ]
            },
        ]
    }
});

Enabling logging here shows the following traces :

I20240227-20:07:34.885(0)? [Subscription.added                 ] users:As3WMvmKfHwBjwJ8L
I20240227-20:07:34.906(0)? [Subscription.added                 ] usersAttributes:8mCfzeCsN4nQn5wLY
I20240227-20:07:34.912(0)? [Subscription.added                 ] usersAttributes:8Hwc2bn9dCQ5Socax
I20240227-20:07:34.913(0)? [Subscription.added                 ] attributes:Ymr9anE4d3rgdgNpr
I20240227-20:07:34.915(0)? [Subscription.added                 ] usersAttributes:XKcTcnCrE6cFG3XK8
I20240227-20:07:34.920(0)? [Subscription.added                 ] usersAttributes:tH5tQfsgTHTomGMsY
I20240227-20:07:34.921(0)? [Subscription.added                 ] attributes:2ekDE7Kj8GCWmJTrD
I20240227-20:07:34.926(0)? [Subscription.added                 ] usersAttributes:ftZ8AHYrxdepSirwv
I20240227-20:07:34.927(0)? [Subscription.added                 ] attributes:kCgmYZuuF6jKt6TYe
I20240227-20:07:34.927(0)? [Subscription.added                 ] attributesChoices:SasZ0077TmKRxQDIfqhFvQ
I20240227-20:07:34.930(0)? [Subscription.added                 ] users:GpK5ZBQE6iCAiMujD
I20240227-20:07:34.930(0)? [Meteor.publish                     ] ready
I20240227-20:07:34.932(0)? [Subscription.added                 ] usersAttributes:kSWETJmvdCeekF86v
I20240227-20:07:34.933(0)? [Subscription.added                 ] attributesChoices:NdL9esNiSlu4YFE5x25BgQ
I20240227-20:07:34.933(0)? [Subscription.added                 ] attributes:WtdS5CrdBjpwFrnLZ
I20240227-20:07:34.937(0)? [Subscription.added                 ] usersAttributes:Auz5fR5vobtrYWKmj
I20240227-20:07:34.938(0)? [Subscription.added                 ] attributes:Lwrb2WK3YkaSXTvYD
I20240227-20:07:34.938(0)? [Subscription.added                 ] attributesChoices:R2NBoPtnTW+YWUahPgg2Kg
I20240227-20:07:34.941(0)? [Subscription.added                 ] usersAttributes:e9YbzBpM9FqcEqoZM
I20240227-20:07:34.944(0)? [Subscription.added                 ] attributesChoices:8YIOwtsCTQSjYLV4HkPzKQ
I20240227-20:07:34.944(0)? [Subscription.added                 ] attributes:yjZA5wwiLjkJBEwid
I20240227-20:07:34.946(0)? [Subscription.added                 ] attributesChoices:5rxoiazjSe6MZwZTeqIb1w
I20240227-20:07:34.946(0)? [Subscription.added                 ] attributesChoices:DW6yWNiARwWf6iTexCaxFw
I20240227-20:07:34.947(0)? [Subscription.added                 ] attributesChoices:WUDi73P5RHaKz/cVH47UXw
I20240227-20:07:34.947(0)? [Subscription.added                 ] attributesChoices:aOThel1MRUiyt/4ABYxajQ
I20240227-20:07:34.950(0)? [Subscription.added                 ] attributes:YPZxX3gpN49Zskg5c
I20240227-20:07:34.950(0)? [Subscription.added                 ] attributes:mbgZcMDZjPa56EWcS

A clear race condition that i narrowed down to this instantiation and how its promises are not awaited here

I created a PR, here.

@StorytellerCZ @reywood , what do you think ?

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

Successfully merging a pull request may close this issue.