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

naturalSelectionStrings.*Property.value is a build error #1314

Closed
pixelzoom opened this issue Aug 27, 2022 · 6 comments
Closed

naturalSelectionStrings.*Property.value is a build error #1314

pixelzoom opened this issue Aug 27, 2022 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 27, 2022

In phetsims/natural-selection#319, I had to undo a whole bunch of work in Natural Selection, because calling naturalSelectionStrings.addMutationsProperty.value (for example) is a build error. All Properties in *Strings.ts are of type TReadOnlyProperty<string>, which includes get and get value() in its API. Why this is an error? Why is it not flagged until build?!? Aren't we trying to eliminate string instances from *Strings.ts eventually?

Also note that getting the value of a StringProperty indirectly, like in this example, is NOT a build error:

class Foo {
 constructor( stringProperty: TReadOnlyProperty<string> ) {
   console.log( stringProperty.value );
 }
}
new Foo( naturalSelectionStrings.addMutationsProperty );

The build error is:

Fatal error: Perennial task failed:
Error: no value for string: addMutations.value
at Object.getStringFromMap (/Users/cmalley/PhET/GitHub/chipper/js/common/ChipperStringUtils.js:148:15)

.. and occurs in getStringMap.js. Assigning to the author @jonathanolson.

High priority because for phetsims/natural-selection#319.

@samreid
Copy link
Member

samreid commented Aug 27, 2022

It thinks “value” is a nested string key. The workaround is to put const m = naturalSelectionString.addMutationsProperty on a separate line, then you can link to m. Someone at dev meeting proposed a long term fix, I think it’s assigned to
@jonathanolson.

@pixelzoom
Copy link
Contributor Author

Thanks. I’ll wait for the longterm fix.

@samreid
Copy link
Member

samreid commented Aug 27, 2022

The general problem is described in phetsims/mean-share-and-balance#99, assigned to @jonathanolson

pixelzoom added a commit to phetsims/natural-selection that referenced this issue Aug 29, 2022
@pixelzoom pixelzoom changed the title naturalSelectionStrings.addMutationsProperty.value is a build error naturalSelectionStrings.*Property.value is a build error Aug 29, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 29, 2022

My workaround in natural-selection ProportionsBarNode.js for phetsims/natural-selection#319:

//TODO https://github.com/phetsims/chipper/issues/1314 using these Properties inline causes a build error
const greaterThanValuePercentProperty = naturalSelectionStrings.greaterThanValuePercentStringProperty;
const lessThanValuePercentProperty = naturalSelectionStrings.lessThanValuePercentStringProperty;
const valuePercentProperty = naturalSelectionStrings.valuePercentStringProperty;

jonathanolson added a commit that referenced this issue Aug 31, 2022
jonathanolson added a commit to phetsims/natural-selection that referenced this issue Aug 31, 2022
@jonathanolson
Copy link
Contributor

Should be fixed above for common usages. @pixelzoom can you verify?

Also, I'd like to note that there are some degenerate examples that the current method will fail on, notably:

        // - joistStrings.someStringProperty[ 0 ]
        // - joistStrings.something[ 0 ]
        // - joistStrings.something[ 'length' ]

We'd need to remove our "error on something that looks like a string but isn't found" since there's no good way to filter these out (without looking for array indices, any method/property that can be on a string, etc.)

Tagging for dev meeting to see if this is acceptable (with a PSA).

@pixelzoom
Copy link
Contributor Author

👍🏻 Confirmed by doing a build of natural-selection with things like naturalSelectionStrings.valuePercentStringProperty.value in the code. Closing.

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

4 participants