Skip to content

Conversation

@seb-cr
Copy link
Contributor

@seb-cr seb-cr commented Jul 18, 2024

The current type is null | undefined, which is unusable!

I've changed this to string | null | undefined as a non-breaking step torwards string | undefined, with plans to remove the null type in v3.

Also improves associated JSDoc.

Jira: ENG-3420

if (queueUrl.includes('.fifo')) {
messageParameters.MessageDeduplicationId = uuid();
messageParameters.MessageGroupId = messageGroupId !== null ? messageGroupId : uuid();
messageParameters.MessageGroupId = messageGroupId ?? uuid();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nullish coalescing treats both null and undefined as "omitted" values.

@seb-cr seb-cr requested a review from corinja July 18, 2024 09:42
Copy link
Member

@corinja corinja left a comment

Choose a reason for hiding this comment

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

Do we know anywhere that existing code is explicitly passing in null and needs to be updated to pass in undefined?

@seb-cr
Copy link
Contributor Author

seb-cr commented Jul 18, 2024

Do we know anywhere that existing code is explicitly passing in null and needs to be updated to pass in undefined?

Yeah, definitely some places (here's an example off the top of my head). However this won't cause any issues until we migrate those projects to TypeScript; the change is backwards-compatible in JS.

@seb-cr
Copy link
Contributor Author

seb-cr commented Jul 18, 2024

Having said that I guess we could preserve null support here, but make it deprecated and drop in v3. Then there's no chance of nasty surprises while still moving towards nicer types.

@seb-cr seb-cr requested a review from corinja July 18, 2024 10:53
Copy link
Member

@corinja corinja left a comment

Choose a reason for hiding this comment

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

Cool. Didn't mean to create more work with my previous comment! But this looks good.

@seb-cr
Copy link
Contributor Author

seb-cr commented Jul 18, 2024

All good; I needed to think about it more and resist breaking semver. It's tempting though when the breaking change is to fix something that was already broken!

@seb-cr seb-cr merged commit 37f7641 into master Jul 18, 2024
@seb-cr seb-cr deleted the ENG-3420/fix-sqs-message-group-type branch July 18, 2024 12:54
@github-actions
Copy link

🎉 This PR is included in version 2.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants