search

phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Enumeration values should be immutable #46

Closed samreid closed 5 years ago

samreid commented 5 years ago

From #42 @pixelzoom said:

  • [ ] You can apparently change Enumeration's values (which are @public, not @public (read-only)) after construction. E.g. 
> var Animals = new phet.phetCore.Enumeration( [ 'COW', 'DOG' ] )
undefined
> Animals.is( Animals.COW )
true
> Animals.VALUES
(2) ["COW", "DOG"]
> Animals.VALUES[0] = 'RABBIT'
"RABBIT"
> Animals.VALUES
(2) ["RABBIT", "DOG"]
> Animals.is( Animals.COW )
false

What was true has now become false.

samreid commented 5 years ago

@jonathanolson said:

Yup, the array should be frozen also.

samreid commented 5 years ago

@pixelzoom said:

Object equality seems to be working nicely. But I can still modify VALUES, it needs to be frozen.

samreid commented 5 years ago

I tested that modifying VALUES indeed throws an error when Object.freeze is set. I added a unit test for this as well. @pixelzoom can you please review?

pixelzoom commented 5 years ago

VALUES is still modifiable, I see no evidence that it has been frozen. And the unit test isn't testing this.

> var Animals = new phet.phetCore.Enumeration( [ 'COW', 'DOG' ] )
undefined
> Animals.VALUES[0]
{name: "COW", toString: ƒ}
> Animals.VALUES[0] = 1
1
> Animals.VALUES[0]
1
jonathanolson commented 5 years ago

Yup, the array should be frozen also.

As in, it SHOULD be done, but it isn't. I added the freezes in the above commit.

Now attempts to modify them properly error:

enumeration-values

pixelzoom commented 5 years ago

And over in https://github.com/phetsims/phet-core/issues/48#issuecomment-444539223, @samreid had said:

Values are frozen as described in #46, unless I'm misunderstanding something. Please clarify.

So I don't know who to believe :)

samreid commented 5 years ago

The miscommunication was reassigning the entire VALUES array vs reassigning elements of the VALUES array. Sounds like both are frozen now. Closing.