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

refactor(connectTo): Call the change handler after value is set #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(connectTo): Call the change handler after value is set #82

wants to merge 1 commit into from

Conversation

jmzagorski
Copy link
Contributor

Change this logic to align with Aurelia's change handling method invocation for
view model properties. @bindable and @observable both call the property
change method after the value is set. This change simply flips the order they are called
in the decorator

closes #69
BREAKING CHANGE: the property value being changed will be set and equal to the
newVal parameter when its change handler is invoked. To migrate from old
code, get the old value from the oldValue parameter in the method call
instead of getting a value from the view model property

Change this logic to align with Aurelia's change handling method invocation for
view model properties. @bindable and @observable both call the property
change method after the value is set.

closes #69
BREAKING CHANGE: the property value being changed will be set and equal to the
newVal parameter when its change handler is invoked. To migrate from old
code, get the old value from the oldValue parameter in the method call
instead of getting a value from the view model property
@zewa666
Copy link
Member

zewa666 commented Nov 4, 2018

Thanks thats great. The problem is still the breaking behavior, so I guess we need to update the docs as well. I'll check this tomorrow and will merge this

@jmzagorski
Copy link
Contributor Author

To be honest, I only did this since I called myself out in #69 to fix this and did not want to keep anyone waiting. However, after I finished and wrote the commit message with the breaking change I doubted a merge since it may bump a major version. Maybe this should wait until a v2.0? It really is not a huge change as you can see.

We could handle #69 in another way since that issue brought up property decorators. I started using typescript and ditched the connectTo decorator in favor of two property decorators i created, @select and @action. In the @select decorator I invoke the change handling methods after I set the value, which follows Aurelia's observable behavior and satisfies #69. This would make the connectTo and select act differently, but with proper documentation that might be okay. What do you think?

@zewa666
Copy link
Member

zewa666 commented Nov 5, 2018

Exactly what I thought. Had a local branch with the same changes and was holding off a bit due to the breaking change. Sooner or later we'll definitely have this merged as V2.

I'd be interested to see what you did with the select and action decorators. Tbvh I pretty much never use connectTo either since I prefer to have full subscription control, so I'm open for any ideas here. Maybe post in the mentioned issue or create a new one as suggestion

@zewa666 zewa666 added the breaking-change This PR introduces a breaking change label Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This PR introduces a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call the changedHandler after the value has been set
2 participants