The name of the extension doesn't feel quite right. The naming methodology of CDK favors having nouns for the classes, and then verb methods on those classes. And I think in general we've tried to keep the same methodology for the extensions too. So it should be something like "service.add(noun)"

I'd suggest something like "Service.add(new IncomingQueue())" or something like that. You may have a better name idea as well, but think of nouns and adjectives for the name, rather than verbs.

Updated the extension name to EventsQueue(), I felt we are already creating a default queue with name eventsQueue for the service so it would be more intuitive to name the extension the same. Let me know if that sounds good!

It might be possible that some folks will not want a dead letter queue. For example if a queue has highly valuable messages going to it, and a message is failing to be processed, they may want to leave it on the queue and fix the code of their queue consuming service, rather than rejecting the message to a dead letter queue. We should make sure dead letter queue is optional

That's a cool one liner!

Do we need to add more docs so that users can at least have some example showing them how to use this feature?

Updated the docs with more guidance on using the extension. Will add a full example once the publish part is over.

I don't get why we need this field. Can't they just specify dead letter queue within eventsQueueProps?

We decided to drop the queue and dead letter queue props completely as they were only being used to instantiate the SQS Queue and served no other purpose. So now if the user doesn't want a queue to be created with the set defaults they need to provide a custom queue as an input prop. This seems to largely simplify the experience as well.

Do let me know if that makes sense or if you have any suggestions!

sgtm

Is it possible to default to eventsQueueProps and make assumptions for the customer if they don't want to customize?

How does this sound? The defaults will be the one's set by CF.

Can we standardize these variable names with Copilot?

For example, we'll use

COPILOT_QUEUE_URI for the primary queue
COPILOT_TOPIC_QUEUE_URIS for a JSON map: {"service-topic": "https://queue-uri.amazonaws.com"}

COPILOT_SNS_TOPIC_ARNS

Could we do something similar and swap COPILOT for ${PARENTSERVICE}?

Actually, the environment in CDK is typed in to be a set of key-value pairs of strings. That sort of limits our ability to add the variables exactly like that.

Update: As the topic-specific queues will be user input, we will be removing the addition of these queueURLs as environment variables. The only queue possibly created by us is the default eventsQueue, and that URL will still be added to the environment.

nit: to illustrate how simple it is to get started, what do you think of updating the first example to:

const myServiceDescription = nameDescription.add(new EventsQueue({
  // Provide list of topics the you want the `eventsQueue` to subscribe to
  topicSubscriptions: [{
     topic: new sns.Topic(stack, 'my-topic'),
  }],
}));

My typescript foo is weak 😅 since we want to have at least one subscription would it make a different to have the following?

I wonder if we can make this even more generic for the future so that it's not only topic subscriptions that we accept but also eventbus subscriptions.

We could create a Susbcribable interface that a TopicSubscription class implements, here is a bad skeleton 😛 : https://gist.github.com/efekarakus/78567b5f4aff33434963dc42080be541

since we want to have at least one subscription would it make a different to have the following?

As this extension is an ‘extension’ 😛 of the current queue processing service (which allows the user to set up a queue and DLQ for their service), I feel we should also support the case where no subscriptions are provided. But if we don’t want to do that then this change would be necessary.

From our discussion, what do you think of having subscribe instead take as input a QueueExtension?

public subscribe(extension: QueueExtension) {
   let queue = extension.eventsQueue();
   if (this.queue) {
       queue = this.queue;
   }
   this.topic.addSubscription(new subscription.SqsSubscription(queue));
}

This way, I think the interface becomes a bit clearer. If a TopicSubscription has a queue then we will use it, otherwise it will be the eventsQueue.

and this would become:

The variable subs is of the type ISubscribable so we first need to do a type check before we can access it’s queue, something like this:

if (subs instanceof TopicSubscription) {
    if (subs.queue) {
      this.subscriptionQueues.push(subs.queue);
    }
}

So it was either the type-check here or dealing with a return in the subscribe method. I feel your suggestion makes the interface look neater, I just wanted to confirm if the type-check was a concern.