{"action":"created","issue":{"url":"https://api.github.com/repos/phetsims/molarity/issues/180","repository_url":"https://api.github.com/repos/phetsims/molarity","labels_url":"https://api.github.com/repos/phetsims/molarity/issues/180/labels{/name}","comments_url":"https://api.github.com/repos/phetsims/molarity/issues/180/comments","events_url":"https://api.github.com/repos/phetsims/molarity/issues/180/events","html_url":"https://github.com/phetsims/molarity/issues/180","id":532371242,"node_id":"MDU6SXNzdWU1MzIzNzEyNDI=","number":180,"title":"Refactor describer listeners to avoid listener dependencies","user":{"login":"twant","id":12232821,"node_id":"MDQ6VXNlcjEyMjMyODIx","avatar_url":"https://avatars3.githubusercontent.com/u/12232821?v=4","gravatar_id":"","url":"https://api.github.com/users/twant","html_url":"https://github.com/twant","followers_url":"https://api.github.com/users/twant/followers","following_url":"https://api.github.com/users/twant/following{/other_user}","gists_url":"https://api.github.com/users/twant/gists{/gist_id}","starred_url":"https://api.github.com/users/twant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twant/subscriptions","organizations_url":"https://api.github.com/users/twant/orgs","repos_url":"https://api.github.com/users/twant/repos","events_url":"https://api.github.com/users/twant/events{/privacy}","received_events_url":"https://api.github.com/users/twant/received_events","type":"User","site_admin":false},"labels":[{"id":322182267,"node_id":"MDU6TGFiZWwzMjIxODIyNjc=","url":"https://api.github.com/repos/phetsims/molarity/labels/dev:code-review","name":"dev:code-review","color":"104E8B","default":false,"description":null},{"id":322345041,"node_id":"MDU6TGFiZWwzMjIzNDUwNDE=","url":"https://api.github.com/repos/phetsims/molarity/labels/status:ready-for-review","name":"status:ready-for-review","color":"3E766D","default":false,"description":null}],"state":"open","locked":false,"assignee":{"login":"twant","id":12232821,"node_id":"MDQ6VXNlcjEyMjMyODIx","avatar_url":"https://avatars3.githubusercontent.com/u/12232821?v=4","gravatar_id":"","url":"https://api.github.com/users/twant","html_url":"https://github.com/twant","followers_url":"https://api.github.com/users/twant/followers","following_url":"https://api.github.com/users/twant/following{/other_user}","gists_url":"https://api.github.com/users/twant/gists{/gist_id}","starred_url":"https://api.github.com/users/twant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twant/subscriptions","organizations_url":"https://api.github.com/users/twant/orgs","repos_url":"https://api.github.com/users/twant/repos","events_url":"https://api.github.com/users/twant/events{/privacy}","received_events_url":"https://api.github.com/users/twant/received_events","type":"User","site_admin":false},"assignees":[{"login":"twant","id":12232821,"node_id":"MDQ6VXNlcjEyMjMyODIx","avatar_url":"https://avatars3.githubusercontent.com/u/12232821?v=4","gravatar_id":"","url":"https://api.github.com/users/twant","html_url":"https://github.com/twant","followers_url":"https://api.github.com/users/twant/followers","following_url":"https://api.github.com/users/twant/following{/other_user}","gists_url":"https://api.github.com/users/twant/gists{/gist_id}","starred_url":"https://api.github.com/users/twant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twant/subscriptions","organizations_url":"https://api.github.com/users/twant/orgs","repos_url":"https://api.github.com/users/twant/repos","events_url":"https://api.github.com/users/twant/events{/privacy}","received_events_url":"https://api.github.com/users/twant/received_events","type":"User","site_admin":false}],"milestone":null,"comments":4,"created_at":"2019-12-04T01:03:02Z","updated_at":"2019-12-19T00:37:56Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"@zepumph and I discussed the below  review item from #171 today, and decided to move away from the current pattern, which we agree could definitely cause some listener dependencies problems. I'll be refactoring all of the describers to move away from these lazyLinks to model Properties. \r\n\r\nItems to check for lazyLinks to model: \r\n- [ ] ConcentrationDescriber\r\n- [ ] SoluteAmountDescriber\r\n- [ ] VolumeDescriber\r\n- [ ] SoluteDescriber\r\n\r\n- [ ] The general algorithm for the lazyLinks in the constructor is to update whenever a value changes. Another way to do this would be to just update when descriptions are asked for. This would mean that the \"granularity\" of the descriptions would only be as good as how often describer methods are called. I don't know that this other way would be better, but one thing I notice is that state in the describer is being updated via those links, and methods all over the sim (like alerts) are calling describer methods. I'm having a hard time guaranteeing in my mind that we don't have a listener order dependency. For example. If you want to alert when the precipitateAmountProperty changes, then you have a listener from that Property calling an alert, AND a listener from that Property updating the concentrationDescriber state to provide the correct describer strings for that alert. I don't yet know enough about how the descriptions in this sim work to know if the above is a problem. Can we talk about this next meeting?"},"comment":{"url":"https://api.github.com/repos/phetsims/molarity/issues/comments/567276128","html_url":"https://github.com/phetsims/molarity/issues/180#issuecomment-567276128","issue_url":"https://api.github.com/repos/phetsims/molarity/issues/180","id":567276128,"node_id":"MDEyOklzc3VlQ29tbWVudDU2NzI3NjEyOA==","user":{"login":"twant","id":12232821,"node_id":"MDQ6VXNlcjEyMjMyODIx","avatar_url":"https://avatars3.githubusercontent.com/u/12232821?v=4","gravatar_id":"","url":"https://api.github.com/users/twant","html_url":"https://github.com/twant","followers_url":"https://api.github.com/users/twant/followers","following_url":"https://api.github.com/users/twant/following{/other_user}","gists_url":"https://api.github.com/users/twant/gists{/gist_id}","starred_url":"https://api.github.com/users/twant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twant/subscriptions","organizations_url":"https://api.github.com/users/twant/orgs","repos_url":"https://api.github.com/users/twant/repos","events_url":"https://api.github.com/users/twant/events{/privacy}","received_events_url":"https://api.github.com/users/twant/received_events","type":"User","site_admin":false},"created_at":"2019-12-19T00:37:56Z","updated_at":"2019-12-19T00:37:56Z","author_association":"CONTRIBUTOR","body":"@jessegreenberg and I investigated this today. When we implemented, we got an error that we believe is caused by a listener dependency issue (where`lastConcentrationValue` is being updated before `concentrationIncreased()` can be called). We looked into a solution for this, which involved updating `lastConcentrationValue` where `concentrationIncreased()` is called, but this did not seem better than the previous solution (since it would have to be updated at multiple call sites). \r\n\r\nIn looking at our previous solution, it also has listener dependency concerns (since the lazyLink function must be called before functions dependent on `this.concentrationIncreased`), but they are not causing issues right now. We agree that it's not ideal, but if we're not going to invest significant time overhauling to address the listener dependency issues so close to RC, it seems like the best solution for now. "}}
{"actor":{"display_login":"twant"}}