ux icon indicating copy to clipboard operation
ux copied to clipboard

Add aurelia-typed-observable-plugin dependency

Open MaximBalaganskiy opened this issue 4 years ago • 14 comments

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

MaximBalaganskiy avatar Jun 09 '20 23:06 MaximBalaganskiy

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

EisenbergEffect avatar Jun 12 '20 04:06 EisenbergEffect

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.

bigopon avatar Jun 12 '20 04:06 bigopon

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

EisenbergEffect avatar Jun 12 '20 12:06 EisenbergEffect

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 avatar Jun 12 '20 12:06 bigopon

And tbh, that plugin matches the theme ux too 😄

bigopon avatar Jun 12 '20 12:06 bigopon

Why not put it right in the core ux package?

EisenbergEffect avatar Jun 12 '20 13:06 EisenbergEffect

hmm yeah that sounds better. Thanks 👍

bigopon avatar Jun 12 '20 13:06 bigopon

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

bigopon avatar Jun 13 '20 08:06 bigopon

Why would it be incompatible?

MaximBalaganskiy avatar Jun 13 '20 08:06 MaximBalaganskiy

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 avatar Jun 13 '20 08:06 bigopon

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.

bigopon avatar Jun 13 '20 08:06 bigopon

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.

EisenbergEffect avatar Jun 13 '20 17:06 EisenbergEffect

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' })
  1. 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 avatar Jun 13 '20 21:06 bigopon

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

bigopon avatar Jun 13 '20 22:06 bigopon