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

Add aurelia-typed-observable-plugin dependency #299

Open
MaximBalaganskiy opened this issue Jun 9, 2020 · 14 comments
Open

Add aurelia-typed-observable-plugin dependency #299

MaximBalaganskiy opened this issue Jun 9, 2020 · 14 comments

Comments

@MaximBalaganskiy
Copy link
Contributor

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
Copy link
Contributor

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

@bigopon
Copy link
Member

bigopon commented Jun 12, 2020

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
Copy link
Contributor

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

@bigopon
Copy link
Member

bigopon commented Jun 12, 2020

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
Copy link
Member

bigopon commented Jun 12, 2020

And tbh, that plugin matches the theme ux too 😄

@EisenbergEffect
Copy link
Contributor

Why not put it right in the core ux package?

@bigopon
Copy link
Member

bigopon commented Jun 12, 2020

hmm yeah that sounds better. Thanks 👍

@bigopon
Copy link
Member

bigopon commented Jun 13, 2020

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
Copy link
Contributor Author

Why would it be incompatible?

@bigopon
Copy link
Member

bigopon commented Jun 13, 2020

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
Copy link
Member

bigopon commented Jun 13, 2020

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
Copy link
Contributor

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
Copy link
Member

bigopon commented Jun 13, 2020

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
Copy link
Member

bigopon commented Jun 13, 2020

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

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

3 participants