C2LC-263: Add feature detection for WebAudio API.

[the-t-in-rtf]

See C2LC-263 for details.

[the-t-in-rtf]

There was an error uploading the flow type coverage artefacts to GitHub:

Error: Unexpected response. Unable to upload chunk to https://pipelines.actions.githubusercontent.com/Gpxshkw4LyVOjgl6K809io2nSsZMKv988USbDqJuOZulHguH7T/_apis/resources/Containers/6593595?itemPath=Flow+Type+Coverage+Report%2Fsourcefiles%2Fsrc%2FC2lcURLParams.js.html

Not sure if someone with more access can run the build, I don't seem to be able to.

[simonbates]

I re-ran the CI build and it passed this time. It seems GitHub CI sometimes has issues uploading artifacts. I've encountered this before on other builds.

[simonbates]

My thinking here is that the code will be easier to maintain if we manage the feature detection in the App and then select whether to create a real AudioManager or a fake one. If we do feature detection at the AudioManager level, the checks against webAudioApiIsAvailable will be distributed throughout the class. For example, we currently have a check on playSoundForCharacterState() but not playAnnouncement(). We don't see an error because the App test doesn't exercise playAnnouncement() but if our App test does more in the future, we'd need to make sure that the AudioManager public API has sufficient checks against webAudioApiIsAvailable.

[simonbates]

There will be some work setting up types. But an example to follow would be the RobotDriver interface:

c2lc-coding-environment/src/types.js

Lines 14 to 19 in 37ce5f8

	export interface RobotDriver {
	connect(onDisconnected: () => void): Promise<void>;
	forward(): Promise<void>;
	left(): Promise<void>;
	right(): Promise<void>;
	};

Which is implemented by https://github.com/codelearncreate/c2lc-coding-environment/blob/production/src/DashDriver.js and https://github.com/codelearncreate/c2lc-coding-environment/blob/production/src/FakeRobotDriver.js

[the-t-in-rtf]

That makes sense, but since switching these is hard coded at the moment for the fake robot driver (and since the speech recognition stuff isn't live either), I don't have a good example of an existing approach.

I assume it's fine to conditionally import based on the feature detection? I'm not sure how this will affect Flow checking, and thought you might know from previous work with feature detection and mocks.

I'll give the interface and fake implementation approach a try, probably tomorrow.

[simonbates]

We could conditionally import, but then we need to deal with waiting until the code is loaded. We have an example of doing delayed loading in a component that we are no longer using (also audio based and checks for AudioContext) which was part of an early experiment trying audio input:

c2lc-coding-environment/src/MicMonitor.js

Lines 27 to 35 in 37ce5f8

	// Import 'react-mic' only if the AudioContext API is available
	if (window.AudioContext || window.webkitAudioContext) {
	import('react-mic').then((reactMicModule) => {
	this.reactMicModule = reactMicModule;
	this.setState({
	reactMicLoaded: true
	});
	});
	}

In this case though, I think we can safely go ahead and import both types and select which to instantiate at runtime. We needed to use to conditional import for the component above because it was creating objects at import. Whereas in our Tone.js case, we are seeing the errors about audio objects missing at construction of the AudioManager.

I'm thinking something like the following:

export default class App extends React.Component<{}, AppState> {
    ...
    audioManager: AudioManager;
    ...
    constructor(props: any) {
        ...
        if (webAudioApiIsAvailable()) {
            this.audioManager = new AudioManagerImpl(this.state.audioEnabled);
        } else {
            this.audioManager = new FakeAudioManager();
        }
        ...
    }
}

Where:

• AudioManager is an interface with the public API for the AudioManager
• AudioManagerImpl is our regular implementation
• FakeAudioManager is a minimal fake implementation of the AudioManager interface

[the-t-in-rtf]

That does seem like a good strategy, I'll make a first pass at that tomorrow.

[the-t-in-rtf]

I had just enough time to make a first pass at this.

[the-t-in-rtf]

Last build failed for code coverage, my guess is I need to exclude the new fake.

[the-t-in-rtf]

The coverage is fixed, the last build failed with a coverage upload error. @simonbates, can you trigger a manual build?

[simonbates]

You've changed a lot of the indentation on this file to 2 spaces. Please use 4 spaces instead.

This is something that ESLint could help us with. I've created a JIRA to add rules to our eslintConfig to check indentation: https://issues.fluidproject.org/browse/C2LC-277

I did a little experimenting this morning and the following seems like a good starting place:

"indent": ["error", 4, {
    "CallExpression": {"arguments": "off"},
    "SwitchCase": 1
}],

This still results in a number of lines that don't match, so there will be some work to do to align the code with the rules.

[the-t-in-rtf]

Yes, I didn't do it on purpose, linting rules are a really good idea. I'll make a small ticket / pull to clean them up.

[the-t-in-rtf]

I'll still add them as part of this pull, actually, but I think a ticket it good to tag the commits with.

[the-t-in-rtf]

Just found C2LC-277, I'll pick up that and use it to tag both the added rules and the cleanup..

[the-t-in-rtf]

I think I'll make a separate small pull after all, there are a couple of edge cases that I'd like to flag and discuss separately.

[the-t-in-rtf]

I temporarily added the rules and fixed the indentation in this file. There's a new pull for the rest.