search

phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

Deprecate PropertySet #102

Closed samreid closed 6 years ago

samreid commented 7 years ago

As discussed in #71 and #101, PropertySet creates more problems than it solves and should be deprecated. The recommended pattern is described here https://github.com/phetsims/axon/issues/71#issuecomment-249094002

with reset and disposal as described here: https://github.com/phetsims/axon/issues/71#issuecomment-249294946

If anyone wants to argue that PropertySet should be kept/fixed/etc, that should happen here. Otherwise, this issue is about how and when we will move away from PropertySet. Some possibilities: (a) Replace all PropertySet occurrences now up front, then delete PropertySet immediately. (b) Leave existing PropertySet occurrences for now, but no new code will use PropertySet (c) When PhET-iO instrumenting a simulation, refactor away from PropertySet

We also have the possibility to refactor PropertySet as @pixelzoom described in https://github.com/phetsims/phet-io/issues/640 as an intermediate solution.

@jessegreenberg @pixelzoom @aadish @andrewadare @jbphet @jonathanolson please comment below and we'll decide at an upcoming developer meeting.

samreid commented 7 years ago

@jonathanolson proposed keeping ES5 get/set in https://github.com/phetsims/axon/issues/71#issuecomment-250016697 and I responded in https://github.com/phetsims/axon/issues/71#issuecomment-250059917

samreid commented 7 years ago

We have to decide in https://github.com/phetsims/axon/issues/71 before doing anything here.

pixelzoom commented 7 years ago

Over in https://github.com/phetsims/axon/issues/71, we decided to move away from PropertySet. So it's constructor should be annotated as @deprectated.

pixelzoom commented 7 years ago

PropertySet constructor and its public public functions as now annotated as @deprecated.

Labeling for developer meeting to decide how to proceed with refactoring away from PropertySet.

pixelzoom commented 7 years ago

The repositories below use PropertySet. Shall we make an issue for each repository?

common code:

simulations:

samreid commented 7 years ago

There are 160 occurrences of PropertySet.call and 20 occurrences of new PropertySet.

pixelzoom commented 7 years ago

I removed PropertySet from simula-rasa, blast, chains and vegas.

samreid commented 7 years ago

@ariel-phet recommended to create 30+ issues, one per each repo. Even better: Let's add a note to the "how to instrument" doc to this effect.

@pixelzoom says we should fix the common code that uses propertyset. @pixelzoom will create issues for that.

@jbphet will tackle ButtonModel.

pixelzoom commented 7 years ago

I'm going to reopen this issue until we actually delete PropertySet.

jonathanolson commented 7 years ago

Can this be un-tagged for developer meeting?

pixelzoom commented 7 years ago

PropertySet has been expunged from common code, and is currently used only in sim-specific repositories. And we decided that we'll expunge from sim-specific code as we instrument for PhET-iO. So yes, looks like we can remove the developer meeting label. And I believe that we can also close this issue.

samreid commented 7 years ago

Hello,

I added a method to Property.js to help in porting away from PropertySet. Consider the PropertySet code:

function SomeConstructor(){
  PropertySet.call(this,{
    name:'test',
    age:123
  });
  ...
}

When converting to Property, you can add this check to make sure you catch all ES5 getter setter references:

function SomeConstructor(){
  this.nameProperty = new Property( 'test' );
  this.ageProperty = new Property( 123 );
  Property.preventGetSet(this,'name');
  Property.preventGetSet(this,'age');
}

Then any code anywhere else that tries to set variable.name or variable.age will throw an assertion.

Best Regards, Sam

pixelzoom commented 7 years ago

Linked to commits related to preventGetSet. I marked it as @deprecated, since it should not appear in production code, and should be deleted once PropertySet is gone.

samreid commented 7 years ago

That seems good, should we also recommend putting it behind asserts so it is stripped for production code?

assert && Property.preventGetSet(...);
pixelzoom commented 7 years ago

... should we also recommend putting it behind asserts so it is stripped for production code?

Why would it be in production code? It's presumably a debugging tool. Developers should add it, run tests, then remove it.

samreid commented 7 years ago

Developers should add it, run tests, then remove it.

Sounds great.

pixelzoom commented 7 years ago

How about moving preventGetSet to PropertySet, instead of Property? Then it will go away when we delete PropertySet. And developers will be encouraged to remove it from their code so that they can remove the var PropertySet = require(...) statement.

pixelzoom commented 7 years ago

I created "convert from PropertySet to Property" issues for all sims that haven't been published yet. For new sims, I don't see any reason why the conversion shouldn't be done before publishing.

pixelzoom commented 7 years ago

Reopening this because @samreid and I seem to still be working on it.

pixelzoom commented 7 years ago

Re https://github.com/phetsims/axon/issues/102#issuecomment-265300832...

How about moving preventGetSet to PropertySet, instead of Property?

Changed my mind - I think it's fine in Property.

pixelzoom commented 7 years ago

Before closing this, let's confirm our decisions regarding "conversion of PropertySet to Property" at the next developer meeting. My understanding is:

• Convert existing sims when they are instrumented for PhET-iO. • Convert new sims before publishing. • Do not create any new code that uses PropertySet.

pixelzoom commented 7 years ago

... and feel free to convert earlier, at developer discretion.

mattpen commented 7 years ago

Developers agree with the bullet points above, there was a suggestion to give this to Denzell as a chip-away task.

pixelzoom commented 7 years ago

I updated the above checklist, by removing sims that no longer use PropertySet.

Whoops, I accidentally deleted the checklist. Someone please paste it back in!

pixelzoom commented 7 years ago

I reviewed current usage of PropertySet, and checked off sims that appear to have been converted. Some progress, but still many sims (24) using PropertySet.

Denz1994 commented 7 years ago

Reference https://github.com/phetsims/projectile-motion/issues/18 for discussion on how most of these can be taken care of.

ariel-phet commented 7 years ago

Note to self, delegate

samreid commented 7 years ago

Bending Light is checked off after the above commits.

samreid commented 7 years ago

CCK basics fixed after the above commit.

samreid commented 7 years ago

Protein synthesis complete.

samreid commented 7 years ago

Finished sugar-and-salt-solutions above.

samreid commented 7 years ago

Energy skate park basics completed, but it was much more complex and hence requires assistance from the QA team when the time is right. I created an issue https://github.com/phetsims/energy-skate-park-basics/issues/371 for that and I'll check it off the list since it is no longer using PropertySet.

samreid commented 7 years ago

The SR sims mentioned in https://github.com/phetsims/axon/issues/102#issuecomment-258203985 are checked off and no longer using PropertySet, so I'll unassign myself.

veillette commented 7 years ago

Curve-Fitting was converted from PropertySet to Property back in March. I closed the issue today (https://github.com/phetsims/curve-fitting/issues/104) .

ariel-phet commented 7 years ago

@samreid put you back as an assignee since there were a couple of sims in your wheelhouse I had not labelled for you before (apologies)

samreid commented 7 years ago

Thanks, it's good to be back!

jessegreenberg commented 7 years ago

rutherford-scattering was done today, https://github.com/phetsims/rutherford-scattering/issues/119 is closed.

jonathanolson commented 7 years ago

Forgot to mark, handled Pendulum Lab today.

samreid commented 7 years ago

I added assignees based on https://github.com/phetsims/axon/issues/102#issuecomment-258203985 but wasn't sure of the GitHub account name for MB

jessegreenberg commented 7 years ago

Thanks @samreid, adding @mbarlow12.

veillette commented 7 years ago

optics-lab was converted to PropertySet (see https://github.com/phetsims/optics-lab/issues/7)

Denz1994 commented 7 years ago

Friction has been converted to Property from PropertySet.

jbphet commented 7 years ago

Area Builder is done.

veillette commented 7 years ago

Charges and Fields is done.

veillette commented 7 years ago

@jessegreenberg and I have completed trig-tour

pixelzoom commented 7 years ago

I did seasons while running some memory leak tests for another sim. "Some intern" will thank me.

mbarlow12 commented 7 years ago

Molecules and Light is converted. See https://github.com/phetsims/molecules-and-light/issues/144.

jbphet commented 7 years ago

Balancing Act is converted, see https://github.com/phetsims/balancing-act/issues/81.

Denz1994 commented 7 years ago

Remaining issues in Neuron were addressed. Reference https://github.com/phetsims/neuron/commit/2045f481459b06904ca75fbe861cfc72ed996605

samreid commented 7 years ago

@ariel-phet said @samreid should get CCK out for testing before @samreid works on this much more.