Skip to content

feat(pg-pubsub): add option to immediately get a result on listen#812

Merged
benjie merged 2 commits intographile:v4from
ol-teuto:pubsub-immediate-listen
Sep 27, 2023
Merged

feat(pg-pubsub): add option to immediately get a result on listen#812
benjie merged 2 commits intographile:v4from
ol-teuto:pubsub-immediate-listen

Conversation

@ol-teuto
Copy link
Copy Markdown
Contributor

@ol-teuto ol-teuto commented Oct 7, 2022

Description

Motivation:

Pretty much the same as #612

subscription MySubscription {
  listen(topic: "user:2", immediate: true) {
    relatedNodeId
    query {
      user(id: "2") {
        email
      }
    }
  }
}

This allows to get a live view of the user email with id 2. Without the option to immediately trigger the query, this would depend on something triggering a NOTIFY first.

This is a non breaking change that adds a feature, it's off by default.

It seems like the tests only check the generated schema, so I only updated that test and didn't add a new one

Regarding documentation, should I open a PR on https://github.com/graphile/graphile.github.io before this is merged?

Performance impact

This is optional and disabled by default, thus there is no performance impact

Security impact

I can't think of a scenario where this impacts security

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link
Copy Markdown
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable feature to add 👍

Comment thread packages/pg-pubsub/src/PgGenericSubscriptionPlugin.ts Outdated
Comment thread packages/pg-pubsub/src/PgGenericSubscriptionPlugin.ts Outdated
topic: {
type: new GraphQLNonNull(GraphQLString),
},
immediate: {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word immediate here is a little confusing - without reading the docs (which don't appear in Explorer, for example), it feels like you should set it because you want to immediately listen ("Why wouldn't I want to listen immediately?!").

We need a name for this that more intuitively encompasses what it does. I'm not super happy with initialEvent because it feels like rather than being a boolean it should be an input object to specify the initial event (I do not want to do this!). But includeInitialEvent/triggerInitialEvent/triggerEventOnStart/withStartEvent/withInitialEvent/eventOnInit/etc feel too wordy. And fireImmediately feels a bit unfriendly.

Having spent a good 5 minutes pacing the living room trying to think of a better name, right now initialEvent is my preferred name for this. But I'm sure there's a better one out there.

Naming things 🤦‍♂️

Suggested change
immediate: {
initialEvent: {

Copy link
Copy Markdown
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, GitHub didn't notify me you'd pushed another commit! Sorry about that 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants