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

Add SQSEvent #58

Merged
merged 3 commits into from
Dec 13, 2021
Merged

Add SQSEvent #58

merged 3 commits into from
Dec 13, 2021

Conversation

fredshonorio
Copy link
Contributor

@fredshonorio fredshonorio commented Nov 20, 2021

Here's the first one, once I get these questions answered I'll hopefully pick up the pace and add more events:

  • It doesn't feel very nice to have all these strings, to what extent should I newtype these? There are also integers represented as json strings (ApproximateReceiveCount)
  • I tested this example json and it seemed to work (although they don't have examples of the message attributes, for example), is this isomorphic enough? or should the runtime representation also be similar to the typescript types?
  • are Map and List ok? I've not done enough scalajs to know if that matters
  • I tend to prefer manually written encoders/decoders, but I can do semiauto if preferred
  • there's no test framework in the project, is there a preference?

@armanbilge
Copy link
Member

armanbilge commented Nov 20, 2021

Thanks for taking this on! And being first, while we figure these things out :)

  1. @bpholt used monix newtypes for the CloudFormation events. I need to think more about this, since that project has only released an 0.0.1. What are the newtypes here? SQSMessageAttributeDataType? Sorry, I derped confusing newtypes with enums.
  2. Let me take a careful look and get back to you.
  3. Yep, all fine! It's unlikely you'll encounter anything that's problematic on Scala.js :)
  4. All power to you! Manual encoder/decoders saves us the shapeless dependency (and improves performance IIUC) which would be excellent :) Edit: also, if you are handwriting these, they don't have to be case classes. Ordinary classes are better for bincompat.
  5. Most Typelevel projects are using munit, so let's use that here :) thanks!

}

final case class SQSRecordAttributes(
awsTraceHeader: Option[String],
Copy link
Member

Choose a reason for hiding this comment

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

@bpholt Now this is interesting!

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, a subtlety I missed is that this trace header is not on the event itself, but rather on each individual record in the event.

@armanbilge
Copy link
Member

armanbilge commented Nov 20, 2021

Oh, regarding integers represented as strings: the JSON spec effectively treats all numbers as Doubles, so using Strings to represent (large) integers is a technique used to avoid loss of precision.

@fredshonorio
Copy link
Contributor Author

Oh, regarding integers represented as strings: the JSON spec effectively treats all numbers as Doubles, so using Strings to represent (large) integers is a technique used to avoid loss of precision.

I understand that part, what I don't know is what kind of types we want after decoded. My laziness pragmatism tells me to not worry about this until the point where I'm using a value, and refactor to the refined type only then, but this might not be a good principle for a library.

@fredshonorio
Copy link
Contributor Author

Later I'll remove the case classes and add the test

@armanbilge
Copy link
Member

I understand that part, what I don't know is what kind of types we want after decoded.

BigInt is safest, or maybe Long if we have guarantees about how big they can get.

@armanbilge
Copy link
Member

is this isomorphic enough? or should the runtime representation also be similar to the typescript types?

So by isomorphic, I mean that the typescript types indicate how this stuff is encoded in the JSON. So our decoders should match the typescript encoding exactly. However, our own classes/models for these events can/should use idiomatic Scala types as much as possible.

@armanbilge
Copy link
Member

armanbilge commented Nov 20, 2021

Regarding newtypes, I'd like to hear @bpholt's and others thoughts on this, since I don't have much experience with them.

However, I'm not yet convinced of their usefulness here. AFAICT newtypes are most helpful when they can be shared across many APIs, otherwise you end up converting between them via the toString representation, which side-steps the type-safety?

For example, it would be nice to use Region from the AWS SDK since I assume that would allow for interop with other libs, but that wouldn't work for Scala.js. If we wanted to go crazy, we could define our own Region that cross-compiles JVM/JS, and the JVM version could have additional methods for converting to/from the AWS SDK Region. That would be typessafe :) but I think that's starting to get out-of-scope.

Edit: there's an Arn type too.

@bpholt
Copy link
Member

bpholt commented Nov 21, 2021

Re: newtypes: IMO, if they can be used by other APIs too, that's nice, but it is not the primary purpose. We used newtypes when creating the custom CloudFormation resource request to help users of the library distinguish at the type level between the different kind of strings present in the input. The PhysicalResourceId and LogicalResourceId are both strings, but it would usually be an error to use them interchangeably. Exposing them as newtypes makes it easier for users of our library to keep them straight if they need to, while not really costing anything for users who don't care (e.g. when logging or whatever).

@armanbilge
Copy link
Member

Exposing them as newtypes makes it easier for users of our library to keep them straight if they need to, while not really costing anything for users who don't care (e.g. when logging or whatever).

Thanks for explaining! That makes sense. I think the only "cost" is the choice of encoding, what do you think about the monix one?

@bpholt
Copy link
Member

bpholt commented Dec 2, 2021

I think the only "cost" is the choice of encoding, what do you think about the monix one?

I don't have a strong preference (which in practice means that some of our internal projects use several different newtype encodings in various places, oops 🤷🏻‍♂️). The Monix one works with Scala 3 and doesn't pull in all of Shapeless.

@armanbilge
Copy link
Member

Seems there were some recent updates to the SQSEvent in DefinitelyTyped/DefinitelyTyped#57488, it now has a return type!

Regarding monix-newtypes, I inquired about its future in monix/newtypes#5.

approximateReceiveCount: String,
sentTimestamp: String,
senderId: String,
approximateFirstReceiveTimestamp: String,
Copy link
Member

Choose a reason for hiding this comment

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

FiniteDuration for this?

@armanbilge
Copy link
Member

@fredshonorio FYI I'll probably merge this as-is tomorrow. I'm pushing to put together a pre-release so people can start playing with feral and I'd much rather have this in than out! We'll keep iterating on these models as we figure out what we want to do about the questions you raised. Thanks for all your efforts!

@armanbilge armanbilge mentioned this pull request Dec 13, 2021
6 tasks
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Our first contributor ❤️ thank you!

@armanbilge armanbilge merged commit f48fa44 into typelevel:main Dec 13, 2021
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.

None yet

3 participants