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

appending a non-existent locale to standard wrapper does not load #274

Closed
Tracked by #892 ...
pixelzoom opened this issue Jan 23, 2023 · 13 comments
Closed
Tracked by #892 ...

appending a non-existent locale to standard wrapper does not load #274

pixelzoom opened this issue Jan 23, 2023 · 13 comments

Comments

@pixelzoom
Copy link
Contributor

https://github.com/phetsims/phet-io/issues/1901 is relevant to phetsims/qa#872 and phetsims/qa#873

@zepumph
Copy link
Member

zepumph commented Jan 24, 2023

Cherry pick: https://github.com/phetsims/phet-io-wrappers/commit/84da4632b21a7163b3e7922604d2ced29b5d4133

Test: follow steps in the first comment of https://github.com/phetsims/phet-io/issues/1901 and make sure a bad locale will work.

@samreid
Copy link
Member

samreid commented Jan 27, 2023

@zepumph and I cherry-picked the fixes and this is ready for QA verification, following the steps described in https://github.com/phetsims/phet-io/issues/1901.

@zepumph
Copy link
Member

zepumph commented Jan 28, 2023

I confirmed in ph-scale and basics 1.6.0-rc.2. QA please feel free to close.

@Nancy-Salpepi
Copy link

Confirming that the standard wrapper opens (bad locale ignored) in ph Scale and ph Scale basics.
However, https://github.com/phetsims/phet-io/issues/1901#issuecomment-1401423861 says that with phetioDebug=true, I should see an assertion when a bad locale is used and I do not. Neither does @KatieWoe.

@KatieWoe
Copy link
Contributor

KatieWoe commented Feb 6, 2023

I attempted this on the standalone wrapper, but phetioDebug=true didn't seem to work. Is this expected?
debugnoton
After discussing with @samreid this aspect is expected. Noting that what @Nancy-Salpepi saw in #274 (comment) also happens on my end.

@arouinfar
Copy link
Contributor

@samreid is it possible that https://github.com/phetsims/phet-io-wrappers/commit/84da4632b21a7163b3e7922604d2ced29b5d4133 didn't make it into the 1.6 branch?

@samreid
Copy link
Member

samreid commented Feb 7, 2023

@KatieWoe and I discussed this in a zoom call. The "standalone wrapper" isn't actually a wrapper and hence doesn't support wrapper query parameters like phetioDebug.

@arouinfar
Copy link
Contributor

The "standalone wrapper" isn't actually a wrapper and hence doesn't support wrapper query parameters like phetioDebug.

Oh, I missed that part of @KatieWoe's comment. However, @Nancy-Salpepi is reporting that there isn't an assertion in the Standard PhET-iO Wrapper for a bad locale when phetioDebug=true. I tested rc.3 and I'm not seeing an assertion, either.

@samreid
Copy link
Member

samreid commented Feb 7, 2023

Thanks, I reproduced the same problem. I saw that in phetsims/beers-law-lab#320 (comment) (5 days ago) @zepumph and I collaborated to improve the locale initialization sequence. It seems it would be better for fallbacks to work in the usual way even if assertions are enabled. So I think we decided we no longer want the behavior listed in https://github.com/phetsims/phet-io/issues/1901#issuecomment-1401423861 (2 weeks ago). Sorry this wasn't clearly communicated. Therefore, this issue can be closed.

@samreid samreid closed this as completed Feb 7, 2023
@arouinfar
Copy link
Contributor

@samreid good to know. We should update phet-io-guide.md because it currently tells clients to expect an assertion.

Relevant section: https://github.com/phetsims/phet-io-sim-specific/blob/master/client-guide-common/client-guide/phet-io-guide.md#use-of-query-parameters-with-the-standard-phet-io-wrapper

If you specify a locale for which a translation does not exist, two things may happen. If phetioDebug=false, the locale query parameter is ignored and the simulation will retain the localeProperty used when creating the Standard PhET-iO Wrapper. If phetioDebug=true, an assertion will be thrown.

@arouinfar arouinfar reopened this Feb 7, 2023
@arouinfar arouinfar assigned arouinfar and unassigned samreid Feb 7, 2023
@arouinfar
Copy link
Contributor

@samreid I updated master in the commit referenced above. The section now reads:

If you specify a locale for which a translation does not exist, the locale query parameter is ignored and the simulation will retain the localeProperty used when creating the Standard PhET-iO Wrapper.

@matthew-blackman
Copy link

Fixed and confirmed in rc.4. @arouinfar said that we can test this ourselves, so we are ready to go to production on that one.

samreid added a commit to phetsims/ph-scale-basics that referenced this issue Feb 7, 2023
@samreid
Copy link
Member

samreid commented Feb 7, 2023

@matthew-blackman and I cherry-picked the change from @arouinfar into ph-scale and ph-scale basics, checked in the RCs and double checked in the production version. 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

7 participants