Deprecate PropertySet

Question

Deprecate PropertySet

Closed this issue 7 years ago · 67 comments

As discussed in #71 and #101, PropertySet creates more problems than it solves and should be deprecated. The recommended pattern is described here #71 (comment)

with reset and disposal as described here:
#71 (comment)

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.

Answer 1 · 2016-09-28T03:11:02.000Z

@jonathanolson proposed keeping ES5 get/set in #71 (comment) and I responded in #71 (comment)

Answer 2 · 2016-09-29T19:06:48.000Z

We have to decide in #71 before doing anything here.

Answer 3 · 2016-10-31T00:17:32.000Z

Over in #71, we decided to move away from PropertySet. So it's constructor should be annotated as @deprectated.

Answer 4 · 2016-10-31T16:54:43.000Z

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.

Answer 5 · 2016-11-03T16:54:57.000Z

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

common code:

joist
scenery-phet
sun
vegas

simulations:

area-builder (JB/DB)
arithmetic (JB/DB)
balancing-act (JB/DB)
balloons-and-static-electricity
bending-light (SR/DB)
blackbody-spectrum
build-a-molecule (JO)
calculus-grapher (JO)
capacitor-lab-basics
charges-and-fields (MV)
circuit-construction-kit-basics (SR/DB)
color-vision (CM)
curve-fitting
energy-skate-park-basics (SR)
faradays-law (JB/DB)
fluid-pressure-and-flow (MK)
forces-and-motion-basics (JG/MB)
fraction-comparison
fraction-matcher (SR)
friction (JB/DB)
gravity-and-orbits (JG/MB)
gravity-force-lab-basics
john-travoltage
least-squares-regression
masses-and-springs
molecule-shapes (DB)
molecules-and-light (JG/MB)
neuron (JB/DB)
ohms-law
optics-lab (MV)
pendulum-lab (JO)
projectile-motion
proportion-playground
protein-synthesis (SR)
resistance-in-a-wire
rutherford-scattering (JG/MB)
seasons (some intern sometime - CM)
sugar-and-salt-solutions (SR)
trig-tour (JG/MB)
wave-on-a-string

Answer 6 · 2016-11-03T16:58:53.000Z

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

Answer 7 · 2016-11-03T17:21:23.000Z

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

Answer 8 · 2016-11-03T18:32:58.000Z

@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.

Answer 9 · 2016-11-18T18:15:58.000Z

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

Answer 10 · 2016-11-30T08:51:34.000Z

Can this be un-tagged for developer meeting?

Answer 11 · 2016-11-30T18:26:58.000Z

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.

Answer 12 · 2016-12-06T18:30:13.000Z

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

Answer 13 · 2016-12-06T19:22:27.000Z

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.

Answer 14 · 2016-12-06T19:25:19.000Z

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

assert && Property.preventGetSet(...);

Answer 15 · 2016-12-06T19:42:13.000Z

... 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.

Answer 16 · 2016-12-06T20:09:13.000Z

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

Sounds great.

Answer 17 · 2016-12-06T22:59:25.000Z

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.

Answer 18 · 2016-12-06T23:13:54.000Z

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.

Answer 19 · 2016-12-06T23:14:22.000Z

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

Answer 20 · 2016-12-08T18:10:32.000Z

Re #102 (comment)...

How about moving preventGetSet to PropertySet, instead of Property?

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

Answer 21 · 2016-12-08T18:13:03.000Z

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.

Answer 22 · 2016-12-08T19:35:29.000Z

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

Answer 23 · 2016-12-08T19:38:13.000Z

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

Answer 24 · 2017-01-19T18:31:39.000Z

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!

Answer 25 · 2017-06-06T22:21:21.000Z

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.

Answer 26 · 2017-06-08T18:00:26.000Z

Reference phetsims/projectile-motion#18 for discussion on how most of these can be taken care of.

Answer 27 · 2017-06-08T18:01:47.000Z

Note to self, delegate

Answer 28 · 2017-06-22T01:35:54.000Z

Bending Light is checked off after the above commits.

Answer 29 · 2017-06-22T01:50:06.000Z

CCK basics fixed after the above commit.

Answer 30 · 2017-06-22T02:37:11.000Z

Protein synthesis complete.

Answer 31 · 2017-06-22T03:08:11.000Z

Finished sugar-and-salt-solutions above.

Answer 32 · 2017-06-22T06:17:27.000Z

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 phetsims/energy-skate-park-basics#371 for that and I'll check it off the list since it is no longer using PropertySet.

Answer 33 · 2017-06-22T06:19:06.000Z

The SR sims mentioned in #102 (comment) are checked off and no longer using PropertySet, so I'll unassign myself.

Answer 34 · 2017-06-22T15:13:00.000Z

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

Answer 35 · 2017-06-25T00:40:18.000Z

@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)

Answer 36 · 2017-06-26T05:32:45.000Z

Thanks, it's good to be back!

Answer 37 · 2017-06-28T22:56:33.000Z

rutherford-scattering was done today, phetsims/rutherford-scattering#119 is closed.

Answer 38 · 2017-06-29T00:42:49.000Z

Forgot to mark, handled Pendulum Lab today.

Answer 39 · 2017-06-29T14:50:22.000Z

I added assignees based on #102 (comment) but wasn't sure of the GitHub account name for MB

Answer 40 · 2017-06-29T15:21:06.000Z

Thanks @samreid, adding @mbarlow12.

Answer 41 · 2017-07-03T22:43:40.000Z

optics-lab was converted to PropertySet (see phetsims/optics-lab#7)

Answer 42 · 2017-07-05T15:23:51.000Z

Friction has been converted to Property from PropertySet.

Answer 43 · 2017-07-05T15:42:53.000Z

Area Builder is done.

Answer 44 · 2017-07-05T21:00:18.000Z

Charges and Fields is done.

Answer 45 · 2017-07-07T22:36:04.000Z

@jessegreenberg and I have completed trig-tour

Answer 46 · 2017-07-07T23:40:16.000Z

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

Answer 47 · 2017-07-07T23:42:28.000Z

Molecules and Light is converted. See phetsims/molecules-and-light#144.

Answer 48 · 2017-07-10T19:37:34.000Z

Balancing Act is converted, see phetsims/balancing-act#81.

Answer 49 · 2017-07-12T17:14:49.000Z

Remaining issues in Neuron were addressed. Reference phetsims/neuron@2045f48

Answer 50 · 2017-07-13T17:50:24.000Z

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

Answer 51 · 2017-07-17T22:56:01.000Z

Arithmetic is now done.

Answer 52 · 2017-07-20T16:18:50.000Z

Faraday's Law is converted. Reference phetsims/faradays-law#77

Answer 53 · 2017-07-21T22:54:52.000Z

@ariel-phet if I end up taking over fliud-pressure-and-flow, I could help in converting it from propertySet if that would be helpful.

Answer 54 · 2017-07-26T16:04:51.000Z

Fraction matcher is all set.

Answer 55 · 2017-07-26T16:10:28.000Z

Fluid Pressure and Flow has 75 Properties, so it will take quite a long time to convert. Before starting on it, I'd like @ariel-phet to comment on @zepumph's note:

@ariel-phet if I end up taking over fliud-pressure-and-flow, I could help in converting it from propertySet if that would be helpful.

@ariel-phet who should work on FPAF? Or should we split it up?

Answer 56 · 2017-07-27T18:54:00.000Z

@ariel-phet asked if @zepumph could work on Fluid Pressure and Flow, I'll reassign him in the checkbox comment.

Answer 57 · 2017-08-03T17:45:51.000Z

@ariel-phet says current priority is "no rush, would be good to be done in a couple of months"

Answer 58 · 2017-08-09T23:16:40.000Z

Does the preceding comment refer to FPAF, or all remaining sims?

Answer 59 · 2017-08-17T17:40:08.000Z

8/17/17 dev meeting: Recollection is that @ariel-phet meant "no rush" on all remaining sims, not FPAF specifically.

Answer 60 · 2017-08-18T20:31:31.000Z

PropertySet removed from Gravity and Orbits.

Answer 61 · 2017-08-24T17:19:53.000Z

PropertySet was removed from forces-and-motion-basics. That was all the sims assigned to @jessegreenberg and @mbarlow12 so removing our assignments.

Answer 62 · 2017-09-12T18:20:48.000Z

I tackled a few spots in Fluid Pressure and Flow this week.

Answer 63 · 2017-09-12T20:04:43.000Z

PropertySet() was converted for Molecules-Shapes. I don't have any more sims to convert so I am removing my assignment.

Answer 64 · 2017-09-13T04:32:31.000Z

Removed PropertySet from Build a Molecule. Past self apparently used new PropertySet( {} ) and trigger/on for what we now use Emitter for.

Removed my assignment.

Answer 65 · 2017-09-14T04:18:25.000Z

I finished removing usages of PropertySet then I deleted PropertySet. I'm going to let Bayes use preventGetSet for a few days in Fluid Pressure and Flow before deleting that. Also leaving this issue open in case there is anything else to discuss about it. Is the next step to "eliminate" Events? #83 Eliminate is in quotes because we may keep it or a variant of it for scenery node.

UPDATE: #142 seems a better reference for emitter/events.

Answer 66 · 2017-09-14T17:08:10.000Z

Agreed, #142 is the reference for converting Events to Emitter.

Looks like our work here is done?

Answer 67 · 2017-09-14T21:31:30.000Z

Good work everyone, that was a biggy!