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

Multilink and DerivedProperty should not allow duplicate dependencies #250

Closed
zepumph opened this issue May 28, 2019 · 6 comments
Closed

Comments

@zepumph
Copy link
Member

zepumph commented May 28, 2019

Discovered while working on phetsims/gravity-force-lab-basics#128.

Right now this "feature" doesn't seem to be used, at least checking via slack.

I also added an assertion like:
assert && assert( _.isEqual( dependencies, _.uniq( dependencies ) ), 'duplicate dependencies' );

to DerivedProperty and Multilink and ran aqua fuzzing with no errors. With this in mind I'm going to commit these changes, and then clean up any issues that may come from this.

@zepumph
Copy link
Member Author

zepumph commented May 28, 2019

Assigning to myself to look at ct in a few hours, then pass off to review.

@samreid
Copy link
Member

samreid commented May 28, 2019

https://stackoverflow.com/questions/28461014/using-lodash-to-check-whether-an-array-has-duplicate-values recommends checking _.uniq(a).length !== a.length, that seems like it could avoid some hassles/headaches with _.isEqual corner cases.

@jonathanolson
Copy link
Contributor

Looks like I'm running afoul of this in proportion-playground, but only conditionally:

  PaintChoice.getActiveColorProperty = function( paintChoiceProperty, side ) {
    var dependencies = [ paintChoiceProperty ].concat( PaintChoice.CHOICES.map( function( paintChoice ) {
      return paintChoice.getColorProperty( side );
    } ) );

    return new DerivedProperty( dependencies, function() {
      return paintChoiceProperty.value.getColorProperty( side ).value;
    } );
  };

Should I wrap a _.uniq around it? Noticed in phetsims/proportion-playground#106.

@zepumph
Copy link
Member Author

zepumph commented May 30, 2019

@samreid implemented above. @jonathanolson that sounds like the right approach.

@zepumph
Copy link
Member Author

zepumph commented May 30, 2019

Implemented above @jonathanolson. Please review. Feel free to close if all looks good.

@jonathanolson
Copy link
Contributor

Looks good, thanks! Closing as requested.

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

No branches or pull requests

3 participants