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

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

Merged
merged 2 commits into from Sep 27, 2023

Conversation

ol-teuto
Copy link
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
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 👍

packages/pg-pubsub/src/PgGenericSubscriptionPlugin.ts Outdated Show resolved Hide resolved
packages/pg-pubsub/src/PgGenericSubscriptionPlugin.ts Outdated Show resolved Hide resolved
@@ -142,6 +174,12 @@ const PgGenericSubscriptionPlugin: Plugin = function (
topic: {
type: new GraphQLNonNull(GraphQLString),
},
immediate: {
Copy link
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
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 😬

@benjie benjie merged commit 12712f0 into graphile:v4 Sep 27, 2023
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.

None yet

2 participants