Swydo / blaze-apollo

Blaze integration for the Apollo Client
MIT License
54 stars 6 forks source link

Add loading property #6

Closed remarcable closed 7 years ago

remarcable commented 7 years ago

Right now, whenever I want to display a query result, I have to safe access the returned objects property.

Example:

const myQuery = gql`
    {
        currentUser {
            favorites {
                title
                artist
            }
        }
    }
`;

// Somewhere in a Template.x.helpers
    favourites() {
        const queryResult = Template.instance().gqlQuery({ query: myQuery }).get();
        const currentUser = queryResult.currentUser || {}; // This is the tiring part
        return currentUser.favorites; // No safe access needed, returns either [] or undefined
    },

This is a bit tiring and repetative. I've seen in several examples (e. g. http://dev.apollodata.com/react/auth.html#login-logout) that there is a loading property for react. What do you think? Would this be great for this package as well?

remarcable commented 7 years ago

Oh. While looking through the source I found the _isReady ReactiveVar. Nice, this means that this feature actually is implemented. I believe though that this is not the cleanest way to do it. isReady could be exposed and the dangling underscore removed. Another option could be doing it the react-way and exposing it in the data object.

What do you think?

jamiter commented 7 years ago

Hi @lightningboss, thanks for your suggestions and sorry about the late response. The result already has the isReady function exposed:

https://github.com/Swydo/blaze-apollo/blob/master/lib/result.js#L27-L29

So I think you can use that for your use case.

Putting it on the data is also an option, but that would require some more code updates, because Result doesn't return the full object, but only the data from that object:

https://github.com/Swydo/blaze-apollo/blob/master/lib/result.js#L49-L54

I think it shouldn't put any loading state on the actual data, because that could conflict when you would request something with a loading field... Right? Looks like the react-apollo package seems to be fine with it though. What do you think?

And there's a third option 😄

What if the .get could receive a path, like .get("currentUser.name")? It would handle the || for you. Probably a separate feature though.

jamiter commented 7 years ago

Btw, if you use currentUser.favorites in the Blaze HTML file itself, it will work right? Blaze will handle the (non-)existence for you.

And another thought:

favourites() {
        const {
          currentUser: {
            favorites: []
          }
        } = Template.instance().gqlQuery({ query: myQuery }).get();

        return favorites;
},

Or:

favourites() {
        const { currentUser: {} } = Template.instance().gqlQuery({ query: myQuery }).get();
        return currentUser.favorites;
},
remarcable commented 7 years ago

I guess you are right. They made the tradeoff of exposing this through in their returning object. Because you are depending on ReactiveVar for the .get() I'd say that's not a feature for this package. There are other ways of doing that in a clean way (like the one you suggested). ;)

I suggest to close this issue. What I would with for though would be a documentation of all methods available (similarly to meteor docs). That would be a separate issue.

jamiter commented 7 years ago

For now I added it to the README. If the API grows larger I'll consider setting up a documentation site for it. It's quite small at the moment so the REAME should be able to cover everything.