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

Switch prop type and prop transform output type? #369

Open
sisp opened this issue Jan 23, 2022 · 3 comments
Open

Switch prop type and prop transform output type? #369

sisp opened this issue Jan 23, 2022 · 3 comments

Comments

@sisp
Copy link
Contributor

sisp commented Jan 23, 2022

While adding the stringToBigIntTransform prop transform, I found it not very intuitive to give the prop the encoded type (e.g. string) and then apply a prop transform to get the desired type (e.g. bigint). Instead, I think the opposite order would be more intuitive, e.g.:

@model("M")
class M extends Model({
  int: prop<bigint>().withTransform(bigintToStringTransform()),
  date: prop<Date>().withTransform(dateToTimestampTransform()),
  // ...
}) {
  // ...
}

What do you think?

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

On the other hand, when using my suggestion with tProp, the input of the prop transform would not be validated whereas with the current approach it is, e.g.:

@model("M")
class M extends Model({
  int: tProp(types.string).withTransform(stringToBigIntTransform()),
  date: tProp(types.number).withTransform(timestampToDateTransform()),
  // ...
}) {
  //
}

Nevertheless, this API feels not so intuitive, at least to me. 🤔

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

Perhaps tProp(...).withTransform(...) could return a new tProp with the new runtime type? Each prop transform would need to provide it, of course. E.g.:

@model("M")
class M extends Model({
  int: tProp(types.bigint).withTransform(bigintToStringTransform()), // -> tProp(types.string).withTransform(bigIntToStringTransform())
  date: tProp(types.date).withTransform(dateToTimestampTransform()), // -> tProp(types.number).withTransform(dateToTimestampTransform())
  // ...
}) {
  // ...
}

Do you know what I mean?

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

The more I think about it, the more it feels related to the other discussions about dropping prop in favor of only using tProp (#299 / #299 (comment), #308).

What I mean is: Property transforms in the general case are not a feature of model props but an encoder/decoder mechanism of runtime types/schemas. Similarly, as discussed in #299, deeply nested optional fields with default values can only be expressed properly using a runtime schema. So I think the ultimate solution to this and the other topics may be building out the runtime types beyond mere type-checking.

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

No branches or pull requests

1 participant