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: add SQS support #34

Merged
merged 26 commits into from
Mar 9, 2024
Merged

feat: add SQS support #34

merged 26 commits into from
Mar 9, 2024

Conversation

skyrpex
Copy link
Contributor

@skyrpex skyrpex commented Jan 31, 2024

  • Resolves SQS support #1
  • Fix pnpm workspace
  • Fix bench:synth
  • Use modern tooling (node 20, tsup, tsx, vitest, ...)
  • Use LocalStack to run tests

@skyrpex skyrpex marked this pull request as ready for review January 31, 2024 10:24
@sam-goodwin
Copy link
Owner

Wow, thanks for the large change. This is awesome!

@skyrpex
Copy link
Contributor Author

skyrpex commented Feb 1, 2024

Wow, thanks for the large change. This is awesome!

No problem! Let me know if you want me to change something, or feel free to do it yourself.

@skyrpex skyrpex marked this pull request as draft February 23, 2024 16:09
@skyrpex
Copy link
Contributor Author

skyrpex commented Feb 23, 2024

I'll check if SQS and SNS clients work...

@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 2, 2024

Hey so it still doesn't work for SNS and SQS. What does it take for it to work, is it on AWS' side? Sorry about the hassle 😢

@skyrpex skyrpex closed this Mar 2, 2024
@sam-goodwin
Copy link
Owner

We will have to check that it is sending the right protocol. Does the JSON api as defined in the types match what the api expects?

No bother! Sorry I haven't been able to help, have been busy!

@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 6, 2024

Spent some hours last night diving into this. Got good news:

  • SQS works without additional work 💪🏻
  • SNS still uses the XML protocol thingy, but can be adapted
  • Prepared the tests to work with LocalStack
  • I think LocalStack has a bug with SNS.publishBatch

I was thinking about repurposing this PR so it runs tests in CI with LocalStack, including SQS. Then, add support for SNS with a custom SNS handler.

@sam-goodwin
Copy link
Owner

Awesome! That's interesting about SNS, what needs to be done to adapt?

Should we re open the PR?

@skyrpex skyrpex reopened this Mar 6, 2024
@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 6, 2024

SNS seems to work the same way as S3 does:

@sam-goodwin
Copy link
Owner

Ah, got it. So only SQS received the JSON API upgrade. Here's hoping 🤞 they do this across the board.

@sam-goodwin
Copy link
Owner

SNS api is probably a lot simpler than S3? S3 had complicated surface area and streaming protocol on top of that.

@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 6, 2024

SNS api is probably a lot simpler than S3? S3 had complicated surface area and streaming protocol on top of that.

I bet! I'll have a look on SNS soon.

@sam-goodwin
Copy link
Owner

Wanna merge SQS? We can release incrementally without much worry.

@sam-goodwin
Copy link
Owner

Don't think I've ever seen a million line PR. Is that because we are checking in AWS models? Maybe we should not check that in?

@skyrpex skyrpex marked this pull request as ready for review March 6, 2024 09:13
@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 6, 2024

Don't think I've ever seen a million line PR. Is that because we are checking in AWS models? Maybe we should not check that in?

Yup, done 👍🏻

Wanna merge SQS?

Sure!

@skyrpex skyrpex changed the title feat: update and fix tooling feat: add SQS Mar 6, 2024
@skyrpex skyrpex changed the title feat: add SQS feat: add SQS support Mar 6, 2024
Copy link
Owner

@sam-goodwin sam-goodwin left a comment

Choose a reason for hiding this comment

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

Hard to identify changes because of formatting updates. But I'm also glad you updated the repo's config so thoroughly lol.

scripts/download-smithy-models.sh Outdated Show resolved Hide resolved
@skyrpex
Copy link
Contributor Author

skyrpex commented Mar 6, 2024

Hard to identify changes because of formatting updates. But I'm also glad you updated the repo's config so thoroughly lol.

Important changes to src/index.ts are:

  • Endpoint resolution. It assumed custom endpoints didn't include the protocol and couldn't use http:// (eg, localstack)
  • Region resolution. It now supports options.region and doesn't crash if env.AWS_REGION isn't defined

The rest is just formatting changes.

@skyrpex skyrpex requested a review from sam-goodwin March 6, 2024 10:28
@skyrpex skyrpex mentioned this pull request Mar 6, 2024
README.md Show resolved Hide resolved
@sam-goodwin sam-goodwin merged commit fdb76bc into sam-goodwin:main Mar 9, 2024
1 check passed
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.

SQS support
2 participants