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

Is ObservableQuery.prototype.setVariables for internal use only? #10788

Closed
1 of 3 tasks
shortercode opened this issue Apr 21, 2023 · 4 comments · Fixed by #10790
Closed
1 of 3 tasks

Is ObservableQuery.prototype.setVariables for internal use only? #10788

shortercode opened this issue Apr 21, 2023 · 4 comments · Fixed by #10790

Comments

@shortercode
Copy link

shortercode commented Apr 21, 2023

First of all I want to say this library is awesome. We have an extensive GraphQL API and I'm directing our project towards using ObservableQuery. In the initial tests I'm seeing some major code improvements and speed gains related to better use of the cache it enables.

At the moment we're having discussions about code patterns before we go deep into the migration. As we are using Stencil.js, not React, we aren't using the hooks API, but instead creating ObservableQuery instances using ApolloClient.prototype.watchQuery.

In case the reader doesn't know; Stencil.js is class based reactive framework that is based on WebComponents. Each component mirrors a custom html element. Decorators are used on class members to define props and state.

Properties are defined by the framework after the class is instantiated, but before any lifecycle hook is triggered. Hence you cannot rely on them being defined in class member initialisers, but will effectively be defined for the rest of the component lifecycle. Hence our issue. We want to define the observable query as a class member, then make the subscription inside the componentWillLoad lifecycle hook. The property that contains the related ID we need for the variables is not available when the query is defined, but is when the subscription is made.

As I see it there are 3 solutions:

  • Define the query in the lifecycle hook. Tradeoff: it makes our code less readable.
  • Use refetch to specify the variables prior to making the subscription. Tradeoff: it skips the cache making the page slower to load.
  • Use setVariables to specify the variables prior to making the subscription. Tradeoff: setVariables is labelled as "internal" and I'm hesitant to rely on it being there if it's not part of the public API.

I'd really like to use setVariables as it gives the easiest to read code and better caching behaviour. Could someone advise on the state of this method and how safe it would be to rely on it going forwards? I've included an example of how we intend to use it for better context.

As an aside while ObservableQuery is awesome, and the documentation for the library is generally excellent, there is relatively little on using watchQuery. Which makes it harder to get people up to speed on how to use the best parts of the library without React. An extra page or 2 here would be a big help!

import { Component, ComponentInterface, h, Prop, State } from '@stencil/core';
import { ContentDetailsDocument, ContentDetailsQuery } from '../generated/graphql';
import { ObservableSubscription } from '@apollo/client/utilities';
import { graphQL } from '../service/graphQL';

@Component({
  tag: 'page-example',
})
export class PageExample implements ComponentInterface {
  @Prop() id_content: string;
  
  @State() content: ContentDetailsQuery['content'];
  @State() loading = true;
  
  private contentQuery$ = graphQL.watchQuery({
    query: ContentDetailsDocument,
  });
  
  private subscription: ObservableSubscription;

  async componentWillLoad () {
    this.contentQuery$.setVariables({ id_content: this.id_content });
    this.subscription = this.contentQuery$.subscribe(({ data, error, loading }) => {
      this.loading = loading;
      this.content = data.content;
    });
  }
  
  disconnectedCallback () {
    this.subscription.unsubscribe();
  }
  
  render () {
    if (this.loading) {
      return <loading-spinner />;
    }
    
    return <h1>this.content.title</h1>;
  }
}

Tasks

@jerelmiller
Copy link
Member

jerelmiller commented Apr 21, 2023

Hey @shortercode 👋 Appreciate the compliments! I'm glad to hear that Apollo is working well for you!

This is a great question!

To be honest, I'm not sure why we label setVariables as internal, especially since its in our documentation. The story gets funnier when you realize that we don't actually use this internally so that comment makes no sense 🤣.

I'd say go ahead and use it! My take is that if we have any API in documentation, its public and can be safely consumed. If we decide to change this API at all, it will be in v4 where we would be making other breaking changes anyways.

Totally agree that "This is for internal use only" should be removed from documentation. I'll get a PR up so others aren't confused by that statement.

As an aside while ObservableQuery is awesome, and the documentation for the library is generally excellent, there is relatively little on using watchQuery. Which makes it harder to get people up to speed on how to use the best parts of the library without React. An extra page or 2 here would be a big help!

You are spot on here! I absolutely agree we need to better document our core API outside React. We have an issue open for it and are aware of it, now its just getting the time to rework our docs. I can't guarantee a timeline on this, but we all agree we should do a better job documenting the core API.

Hope this helps!

Edit: Also wanted to say I appreciate the thoroughly worded issue! It made it easy for me to understand the reason you find this API desirable and where we could be doing a better job in our documentation. Thanks so much!

@shortercode
Copy link
Author

Awesome, thanks for taking the time @jerelmiller

I kinda assumed that it was only in the documentation because that doc page look like it was generated from the source code. But as you say, if it's public and documented its fair game right? 😆

But yeah thats great news, means we can go ahead. I'll pass the news onto the others in the team.

Out of interest is there a v4 in the works? I'm sure we can manage the transition if/when it comes but I'm excited to hear what sort of improvements the team has planned.

I managed to piece it together the core API usage from the hooks documentation and a bit of guesswork. But as I say it'd be great if people from other frameworks had a bit more of a hand.

I'm in the process of writing internal documentation about best practises for GraphQL anyway so if there's anything I can offer to improve the official documentation just let me know.

@jerelmiller
Copy link
Member

I kinda assumed that it was only in the documentation because that doc page look like it was generated from the source code. But as you say, if it's public and documented its fair game right?

That was a great assumption given the way that page is structured 😆. You're correct in that it is generated from source code, but there is still a manual component in order it to show up in the documentation. Because of this, I view this as an intentional decision to add it to our documentation rather than something that just happens to show up automatically.

Out of interest is there a v4 in the works?

I'd say yes and no.

Yes in the fact that we do have a v4 branch with some changes on it.

No in the fact that I'd still say we are a ways off before considering any kind of release. We certainly have nothing to show off anytime soon and aren't actively working on v4 changes right now.

We don't yet have a cohesive story on what v4 will look like. Internally we've ramped up talks on what we'd like to see with v4, but we don't have much publicly to share yet. Right now this is mostly just a wishlist of ideas and random thoughts.

FYI take that v4 branch with a grain of salt. We may or may not alter it by the time we are actually serious about v4. It helped us track some breaking code changes we've wanted to make, but that branch is actually pretty stale (last commit was 7 months ago). We just like to communicate now that we won't consider breaking something until the next major version, even if that is still some ways off.

Once we get some more cohesive thoughts around v4, we'll do our best to share that around!

if there's anything I can offer to improve the official documentation just let me know.

Absolutely! If you want to help improve our docs in any capacity, feel free to submit PRs! We love it when we get external contributions on our docs to improve them. We'd be happy to review them!

Just a word of advice here, if you're wanting to help contribute larger changes (such as an entire page or something), you might consider opening an issue before putting in the work to write everything out, just to make sure the idea gels with what we are thinking. Not sure that's what you're asking, but figured I'd point it out. I just wouldn't want you to put in hours or days of work on a docs page that we end up closing because it doesn't fit with our docs.

For smaller changes, no need. A PR is totally fine!

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants