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

[0.8]: observer callbacks changed parameter ordering #1363

Closed
ebidel opened this issue Apr 2, 2015 · 12 comments
Closed

[0.8]: observer callbacks changed parameter ordering #1363

ebidel opened this issue Apr 2, 2015 · 12 comments
Labels

Comments

@ebidel
Copy link
Contributor

ebidel commented Apr 2, 2015

0.5: disabledChanged: function(oldValue, newValue) {

0.8: disabledChanged: function(newValue, oldValue) {

It's a very subtle change and easy to miss...adds "one more thing to do" when porting 0.5 code :(

Was there a reason to change the ordering?

@ebidel ebidel added the 0.8 label Apr 2, 2015
@Jeff17Robbins
Copy link

Since the order in attributeChangedCallback is oldValue, newValue, it would
seem that the 0.5 order was more consistent, and so more memorable. So
that's a reason to NOT change the ordering and leave it as it is in 0.5.

On Thu, Apr 2, 2015 at 1:58 PM, Eric Bidelman [email protected]
wrote:

0.5: disabledChanged: function(oldValue, newValue) {

0.8: disabledChanged: function(newValue, oldValue) {

It's a very subtle change and easy to miss...adds "one more thing to do"
when porting 0.5 code :(

Was there a reason to change the ordering?


Reply to this email directly or view it on GitHub
#1363.

@ebidel
Copy link
Contributor Author

ebidel commented Apr 2, 2015

+1 also good point.

@justinfagnani
Copy link
Contributor

FWIW, newValue as the first argument is much more convenient for use with one-arg functions.

@ebidel
Copy link
Contributor Author

ebidel commented Apr 2, 2015

I haven't seen many people use Changed handlers as callbacks. 90% of the time you just use this.disabled to get the latest value.

@sjmiles
Copy link
Contributor

sjmiles commented Apr 3, 2015

Would prefer all official elements going forward to use:

fooChanged: function(foo) {
}

style.

But the lack of parallelism wrt attributeChangedCallback is a bit of a wrinkle.

@morethanreal
Copy link
Contributor

Why do you prefer to use the parameter instead of this.foo?

@sjmiles
Copy link
Contributor

sjmiles commented Apr 14, 2015

It's more compact and less stateful. Parameterization is generally good factoring.

@jlg7
Copy link

jlg7 commented Apr 15, 2015

if it is implicit that this.foo is always foo when the observer callback occurs, then isn't foo just more state? Doesn't 0.8 require explicit observer properties configuration which may heighten how stative it is?

<script>
    FooChanges = {
        properties: {
            foo:{
                type:String,
                value:"orderMatters!",
                observer:"fooChanged"
            }
        },
        fooChanged:function(){
            this._biasTowardsTheNew(this.foo);
        },
        _biasTowardsTheNew:function(foo){
           console.log("Communicating about how to communicate change is difficult");
        }

    }
</script>

Perhaps exposing an additional configuration parameter when annotating a property observer might be useful to appease single arg new bias 0.8 and existing 0.5 styles. A default can be decided over a thumb wrestling match.

@arthurevans
Copy link

I think Scott's point was that if the order is new, old, then you can use a single-argument observer, too. That is, in your example, observer could simple point to _biasTowardsTheNew directly.

@jlg7
Copy link

jlg7 commented Apr 15, 2015

@arthurevans thank you for emphasizing this and have no argument for those who have that preference. I was addressing the parameter foo being more stateless and compact than this.foo in the context of a polymer element with a properties object and explicit observer configuration. It seems like it adds more to the mix to me. I am definitely PRO single argument observer but seems to be more of a preference when setting up your observations.

@sorvell
Copy link
Contributor

sorvell commented May 22, 2015

A function that does not include this is truly stateless. In general, an observer that uses an argument name rather than a specific property can be used to observe different properties.

@sorvell sorvell closed this as completed May 22, 2015
@jlg7
Copy link

jlg7 commented May 22, 2015

TYVM! I absolutely agree with stateless function definition and ease of reuse! Just reiterating, the polymer/behavior declaration has no additional state after the observer callback using this assuming no state is added in the implementation compared to the argument approach. Different element properties of the same name seem possible using the this approach and not sure if that would be the frequent case.

Additional decoupling in a behavior or in an element instance may occur after the properties are specifically mapped to an observer. A single property observer when attached to n properties of the same element instance fires n times when one of them is changed with no way to differentiate. I believe this does require some delegation in the implementation already to have reuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants