Sounds require simLauncher, even when imported outside of sims.

[zepumph]

I encountered this while trying to make a small change to CT over in phetsims/aqua#109. When I build aqua as a standlone module, it imports common code elements (like checkbox) that have sound in them. These sounds import by putting a lock on simLauncher. Thus, a non-sim (and potentially non phet) module depends on joist. This feels like a large flaw to me. That said I don't know a way around this. Marking for dev meeting to see if others feel like this is an issue.

This issue is mostly buggy because when you build the continuous report and load it, it errors out because it can't find the "Brand" module for the default brand. Continuous report should not have a brand.

I thought I could fix with this band-aid, but to no avail:

Index: js/aqua-main.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/aqua-main.js	(revision 749f51ce792cb5039e6a2013d6ecc8ea226b2ab8)
+++ js/aqua-main.js	(date 1604691220782)
@@ -1,3 +1,4 @@
 // Copyright 2020, University of Colorado Boulder
 
+import '../../brand/adapted-from-phet/js/Brand.js';
 import './report/report.js';

This does not work. And even if it does (which I can hack around by setting the brand manually with ?brand=phet, there are downstream dependencies on things like phet.chipper.strings. None of these should be required for a stand alone scenery build that uses common code elements that happen to load sound.

Can we get rid of a forceful joist dependency in our common code?

[zepumph]

This effects the production version of CT, so I feel like it should be high priority.

[zepumph]

With phetsims/aqua@d250548, there are no errors when loading continuous-report.html anymore, but these are just workarounds for this issue.

[pixelzoom]

12/10/2020 dev meeting

@jbphet Asked why we're using sound outside of simLauncher context. @zepumph gave the example of using sun UI components in CT.

@jonathanolson @zepumph pointed out that someone using sun (which is supposed to be "general") is going to have problems.

@jonathanolson suggested testing builds of repos like sun, to verify that they don't have undesirable dependencies.

@zepumph workaround for non-joist applications is to add dependencies that are need by sound (but not by the application).

@jbphet @zepumph @samreid @jonathanolson will work on decoupling sound from repos (like sun) that shouldn't be dependent on sound.

Priority is low, to be addressed after the new year.

[zepumph]

• I will set a calendar reminder to bring this back up in the beginning of january
• I will create an issue in aqua to add node testing support, including a way to build common code sims (like sun) and make sure that it doesn't depend on joist or sims.

[samreid]

Self-unassigning for now, will reassign once we discuss how to decouple sound.

[zepumph]

Meeting has been scheduled for next Friday, 1/22

[zepumph]

@jonathanolson, @samreid, @jbphet, and @zepumph discussed this issue together today. The main focus of it ended up being more about joist in general as a dependency that should be leaking into common code that doesn't need a Sim to exist. The primary places we see this happening are Tambo and Sun, though scenery-phet could likely have some cleanup too, even though it is more closely tied to phet sims.

I have created some side issues already to discuss this work, see phetsims/joist#686, and phetsims/joist#685.

Also @jonathanolson is going to create a new scenery issue to discuss adding an inputListener to ALL displays, which will help a case of joist leaking around TAMBO/DisplayedProperty

From here in this issue, I will focus on the decoupling of simLauncher's "locks" from common code, which we spent a large amount of the discussion time we had deliberating. Here is the plan we came up with:

• Create PHET_CORE/asyncLoader.js to handle all the locks be used by simLauncher and elsewhere.
• This way image/sound files can import asyncLoader instead of simLauncher (from joist)
• keep simLauncher.launch( () => { as the api in *main.js files. (which would forward asyncLoader.launch)

This will require us to re-modulify every file, what fun!

[zepumph]

I just had @samreid give me a working-copy review of asyncLoader.js and simLauncher.js. Things are looking good, and I'm getting ready for a commit. See phetsims/joist#687 for the commits.

[zepumph]

@samreid please finish off the review for this work in this issue (important files linked here), and take a few looks around commits in phetsims/joist#687.

During our discussion you said you wanted an assertion like

assert && assert( !this.readyToProceedCalled, 'readyToProceed has been called, so no more locks can be added.' );

at the top of asyncLoader.createLock, but that didn't work for phet sims out of the box (it triggered the error). I still feel like this is alright because the callback hasn't yet been called or checked on. But is seems like a code smell. Do you have any thoughts about it?

Anything else here?

[zepumph]

Sorry @samreid, I missed pushing that last phet-core commit for a couple hours. I think this review should block publication, but it should be a quick one.

[samreid]

It seems the problem from #126 (comment) is that simLauncher is calling asyncLoader.readyToProceed before the brand or phet-io engine are unlocked. It seems we should wait for those to unlock before calling readyToProceed. To implement this, it seems simLauncher will need its own sort of "locks" to know when to give the go-ahead. Does that sound good to you?

[samreid]

Also, it seems like readyToProceedCalled should just be another lock--without needing separate treatment.

[samreid]

I believe these commits address both of the preceding comments. I tested in both unbuilt and built and didn't see problems, but I think it should still warrant "blocks-publication" until @zepumph can review. If RC SHAs are need imminently, it may be safe enough to sanity test and take SHAs, keeping in mind that improvement "cherry-picks" may come from this issue.

[samreid]

CT was failing unit tests because they have no asyncLoader listener. I removed the assertion--not sure if that's best or if something should be listening. Please review.

[zepumph]

What a very fun and clever approach! (clever in the good way)

I feel like this addresses all of our concerns. I'm not worried about ensuring that there are listeners. asyncLoader can be flexible in that way, as its a low level library tool.

I'm ready to close if you are.

[samreid]

Excellent, thanks! Closing.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[samreid]

The TODO is:

  // TODO: remove these workarounds once https://github.com/phetsims/tambo/issues/126 is fixed
  window.phet.chipper.strings = {
    en: {
      'JOIST/translation.credits.link': {
        value: 'Translation Credits'
      },
      'JOIST/thirdParty.credits.link': {
        value: 'Third-party Credits'
      }
    }
  };
  window.phet.chipper.brand = 'phet';

I'm not certain how to test or verify whether that is still needed. That code was added in phetsims/aqua@d250548. @zepumph can you please take a look?

[zepumph]

I didn't totally understand how the fix works in this issue, but I traced back to the workaround commit I added originally, and removed it, then built aqua and tested with this link:

http://localhost:8080/aqua/html/continuous-report.html?server=https://sparky.colorado.edu

Things work well. I'm ready to close. Thanks!