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

Why don't store models leverage property descriptors too? #209

Open
cyruseuros opened this issue Jul 15, 2023 · 7 comments
Open

Why don't store models leverage property descriptors too? #209

cyruseuros opened this issue Jul 15, 2023 · 7 comments
Labels
store Applies to the store feature

Comments

@cyruseuros
Copy link

I'm sure there's a good technical reason - I just find myself reaching for the niceties of custom elements with store models, but they're just not there. E.g. instead of the power of setter functions we have store.validate that can only reject invalid inputs.

@smalluban
Copy link
Contributor

I think I don't fully understand your case. Can you provide an example code, which show the problem?

@smalluban smalluban added the store Applies to the store feature label Jul 16, 2023
@cyruseuros
Copy link
Author

For instance it would be more powerful if instead of:

const Set: Model<Set> = {
  id: true,
  weight: 0,
  distance: 0,
  reps: 0,
  time: 0,
  angle: store.value(
    0,
    (v) => v >= 0 && v <= 90,
    'Only angles of 0-90 degrees are supported',
  ),
}

I could write:

const Set: Model<Set> = {
  id: true,
  weight: 0,
  distance: 0,
  reps: 0,
  time: 0,
  angle: {
    get: ...
    set(host, value) => {
      if (ok(value)) {
        return value
      }
      if (recoverable(value) {
        return fix(value)
      }
      
      throw new Error('oops')
    }
  },
}

@cyruseuros
Copy link
Author

Thanks for taking so much time btw - I'm still trying to learn hybrids to a level where I can actually implement an application in it so I might be missing some rather obvious stuff. That being said I really like the model (and 0-dep approach) thus far and I figure it's best to surface these things to make it easier for people when hybrids makes it big ;)

@smalluban
Copy link
Contributor

smalluban commented Jul 20, 2023

The store models rely on the value type, and by the design, they must be serializable to JSON-like structure. The store would not be able to detect (or even check) if the property defined with get/set methods fits the requirements.

However, you are free to make that validation in your set method. If you return a proper structure, it can be even used with store.error() helper, to show an error in right place:

const Model = {
   value: 0,
   [store.connect]: {
      get() { ... },
      set(_, model, keys) {
        if (keys.includes('value') && !validate(model.value)) {
           const err = new Error("Validation error");

           err.errors = {
             value: "Value must be ...",
           };

           throw err;
        }
        ...
      },
   },
};

You can find more info about throwing errors here: https://hybrids.js.org/#/store/usage?id=storeerror

Additionally, you are free to create a computed property, which returns whatever you want, as this is not serialized (I know, that this is not what you mean here, but I say it to highlight what's possible).

@cyruseuros
Copy link
Author

Makes sense from a design perspective, though it is a tad cumbersome from a dev perspective since there are now 2 spots where you handle any given value in a store (its own property and the store.connect property).

Perhaps a better place to handle this is in an exported setter "action" function similar to what Pinia has? Am I correct in assuming those won't need special framework support and essentially amount to a design pattern consisting of an exported function from whichever module you implement your store/model in? If so, is there any way to ensure that the field is only set through that function (private fields are not valid inside POJOs)?

Is this a common restriction (being JSON serializable)? I suspect so, I've just never delved into state management library internals. I'm only asking because I don't recall bumping into this problem.

PS: Do you guys have a place where you have more casual discussions that don't need issue tracking? E.g. I would like to slowly start contributing typescript "translations" for all the docs in and would like to sync up on the best approach to doing that. Or asking silly questions about routing patterns etc.

@smalluban
Copy link
Contributor

I have never thought before about adding to store models setters of the properties. It is something that I must rethink how it would be possible to integrate with the current API. One thing that comes to my mind is to extend store.value() function, so it could not only validate but also return a computed value.

The [store.connect].set method takes a frozen instance of the changed model. It makes it harder to make any change to that model in this function - but this is intentional - the set function is to send an instance to the data source, not make any change to the model itself (still the source can return changed model, so it is updated when returned).

Is this a common restriction (being JSON serializable)? I suspect so, I've just never delved into state management library internals. I'm only asking because I don't recall bumping into this problem.

This is also an architecture decision to make it easier to send over the wire, send to different types of data sources. The store models are not intended to be complex structures, but they support computed properties, which can return anything - and they are not sent when JSON.stringify() is called on the model instance.

@cyruseuros
Copy link
Author

Personally I think people are far more used to the concept of "action" functions - see all the "increment" examples in different state management libraries. The kicker is figuring out how to make the properties only editable through those (probably through some sort of opt-in mechanism). + it's far more flexible.

That said, looking forward to whatever you come up with should you choose to work on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
store Applies to the store feature
Development

No branches or pull requests

2 participants