search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Assert that all provided/supported portrayal instances are the same in preferences and package.json values #950

Closed zepumph closed 5 months ago

zepumph commented 5 months ago

From https://github.com/phetsims/joist/issues/949, I found a potential bug that I think could use an assertion. Currently there is nothing making sure on startup that all provided portrayals match up with the package.json supportedRegionsAndCultures. Without an assertion near this spot in the preferences model, you run the risk of being able to provide a query parameter value that doesn't align with the available portrayals from the preferences.

https://github.com/phetsims/joist/blob/22c3231bc37c6618ffaa6cc635139927db9369a4/js/preferences/PreferencesModel.ts#L316-L326

I'd like to add this assertion and ask @marlitas to review.

zepumph commented 5 months ago

@marlitas please review. I actually found one bug that I had been hoping to cover as part of this (at least I think it was a bug) in ESPB. Can you please let me know what you think?

marlitas commented 5 months ago

@zepumph hmmm we never actively worked on ESPB but I guess it grabs code from ESP which is why it does support some of that now? Yeah that's a tricky scenario... Adding it to the package.json because of that is probably the right call.

The assertions look good and I think this will be very helpful for future scenarios like this in the future. Thanks @zepumph! Closing.