Skip to content

use simple ADT to encode attribute values #27

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

Closed
wants to merge 2 commits into from
Closed

Conversation

buntec
Copy link
Owner

@buntec buntec commented May 9, 2022

A basis for discussion.

@armanbilge armanbilge linked an issue May 9, 2022 that may be closed by this pull request
case class StringAttrValue(value: String) extends AttrValue
case class BooleanAttrValue(value: Boolean) extends AttrValue

implicit def stringToAttrValue(value: String): AttrValue =
Copy link
Owner Author

Choose a reason for hiding this comment

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

I know this is controversial but still seems to be quite common for such cases, see e.g., here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. I think it goes to @cornerman's question in #14 (comment)

Do we consider this library directly user facing or is it more like a low level library powering, e.g., ff4s and outwatch.

If it's a low-level library, we probably wouldn't need this, right?

@cornerman
Copy link

In general this looks good, but i am a bit concerned about allocations - introducing a wrapper for each attribute value. Does it have advantages over Boolean | String apart from userfriendliness? :)

@buntec
Copy link
Owner Author

buntec commented May 14, 2022

I hear you. I would prefer, however, not to use js.|, just like I have been avoiding js.Array, js.UndefOr etc. My impression is, perhaps incorrectly, that they should be reserved for interop scenarios. They also take away from the pure Scala feel in my opinion, which is very subjective of course.

In any case, it would be great to have a set of benchmarks to be able to measure these things so that we can make an informed decision.

@cornerman
Copy link

I understand and totally agree that we need benchmarks for a good decision 👍

For me, I am actually using quite a lot of native types (like js array), because they should be quite optimized in today browsers - and served me well when measuring performance in a huge webapp developed with outwatch.

@armanbilge
Copy link
Collaborator

armanbilge commented May 14, 2022

Another strategy here, for keeping a friendly user API while avoiding allocations could be to use an encoding like Akka's OptionalVal.
https://github.com/akka/akka/blob/30749205a214e6e38fafe2636ea685b4c775fff2/akka-actor/src/main/scala/akka/util/OptionVal.scala

Internally, we would have:

class AttrValue(val v: String | Boolean) extends AnyVal

and then expose the ADT "members" as apply and unapply methods.

@buntec buntec closed this Jul 4, 2022
@buntec buntec deleted the adt-for-attr-values branch July 4, 2022 18:40
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.

Boilerplate trade-offs in APIs
3 participants