aurelia / ux

A user experience framework with higher-level capabilities, designed to bring simplicity and elegance to building cross-device, rich experiences.
MIT License
368 stars 55 forks source link

Add aurelia-typed-observable-plugin dependency #299

Open MaximBalaganskiy opened 4 years ago

MaximBalaganskiy commented 4 years ago

It would be great to use aurelia-typed-observable-plugin in UX components. At the moment, whenever we need a boolean or a number property, a lot of plumbing code is needed to convert strings to typed values.

E.g.

  debounceNumber: number | null | undefined = this.defaultConfiguration.debounce;
  @bindable
  debounce: number | string | undefined = this.defaultConfiguration.debounce;
  debounceChanged() {
    this.debounceNumber = normalizeNumberAttribute(this.debounce);
  }

As you can see, far from convenient. Plus, I need to remember to use a shadow property instead of a bindable directly.

With the plugin this becomes just

  @bindable // this is plugin's bindable
  debounce: number = this.defaultConfiguration.debounce;

The advantage is obvious

EisenbergEffect commented 4 years ago

I'd prefer to add this to the core and do it in a way compatible with Aurelia 2. @bigopon Is that doable?

bigopon commented 4 years ago

It's doable, all the patches needed are here https://github.com/aurelia-contrib/aurelia-typed-observable-plugin/blob/master/src/patches.ts

Though aren't we constraining ourself from adding new features to v1? Also, at the moment, v2 coerce is `set:

@bindable({ set: v => v })

but the plugin mentioned is coerce:

@bindable({ coerce: v => v })

Yeah, it's probably mainly about releasing feature to v1, also the ramification of this as well, as Jeremy commented elsewhere. I think it's better in the form of a plugin. Can make it official part of ux if needed.

EisenbergEffect commented 4 years ago

What if we move the plugin into ux but renamed it so that the APIs matched the proposed behavior of v2?

bigopon commented 4 years ago

I can do that, but probably it will be a new package, as it doesn't seem to be simply "move" a plugin over a monorepo. Sounds like you are happy with that, I'll proceed that way

bigopon commented 4 years ago

And tbh, that plugin matches the theme ux too 😄

EisenbergEffect commented 4 years ago

Why not put it right in the core ux package?

bigopon commented 4 years ago

hmm yeah that sounds better. Thanks 👍

bigopon commented 4 years ago

On a 2nd thought, doing it in UX will make ux incompatible with that plugin in user app. How should we handle that situation?

MaximBalaganskiy commented 4 years ago

Why would it be incompatible?

bigopon commented 4 years ago

In order for this to works, it needs to override setValue of the class BehaviorPropertyObserver in templating. And when both of them try to override the same method with their own compatible version, it'll break.

bigopon commented 4 years ago

Actually there's a way to make it not break, via duck typing. Means that this implementation must be exact to the other in terms of naming for internal bits.

EisenbergEffect commented 4 years ago

We may want to make an exception and just add this to aurelia-binding anyway. I think it would be ok since this will also bring forward compat with au2 while solving an issue in UX.

bigopon commented 4 years ago

It would be difficult because we haven't locked down how the API would be.

  1. In v2:

    @bindable({ set: v => v })

    And potentially:

    @bindable({ mode: 'toggle' })
    @bindable({ mode: 'date' })
    @bindable({ mode: 'string' })
    @bindable({ mode: 'number' })
  2. in the plugin:

    @bindable({ coerce: v => v })
    @bindable({ coerce: 'toggle' })
    @bindable({ coerce: 'boolean' })
    @bindable({ coerce: 'date' })

If we are to add this to binding, at least need to decide how the enhancement on those APIs would be first.

bigopon commented 4 years ago

I think for now we make an exception and use the plugin, until we lock on how we enhance the API for bindable/observable decorators in v2 and we move this over