Add helper libraries to App Lab

[joshlory]

Make it possible to load helper libraries on App Lab levels:

[islemaster]

I've got some questions and some possible cleanup that could be deferred to a future PR.

[islemaster]

Nit: I prefer the conventional resolve, reject parameter names, mostly because reject is definitely a verb while error could be a noun. IMO it also reads better in error-handling cases, as when wrapping a node-style callback in a promise.

new Promise((resolve, reject) => {
  asyncWork((err, result) => {
    if (err) {
      reject(err); // A little clearer than `error(err)`
    }
    resolve(result);
  });
});

I could be convinced that in this case, where you're passing it directly as the error handler, onError would be a better name than reject.

[islemaster]

If libraries is on the instance, it seems like the libraryPreload promise should also live on the instance instead of in a static context.

[islemaster]

Did you consider writing these methods with memoize and fetch and async/await? (no promises I got all the below syntax right)

StudioApp.prototype.loadLibraries_ = _.memoize(function (helperLibraryNames = []) {
  return Promise.all(helperLibraryNames.map(name => this.loadLibrary_(name)));
});

StudioApp.prototype.loadLibrary_ = _.memoize(async function (name) {
  const response = await fetch(`/libraries/${name}`)
  this.libraries[name] = await response.text();
});

[joshlory]

Cool, TIL about _.memoize! Good call using fetch over $.ajax, I need to start getting in that habit.

[davidsbailey]

+1, TIL about _.memoize! Question for either of you: what is the benefit of fetch over $.ajax?

[islemaster]

Fetch is (almost) standard. Although not part of ECMAScript, the Fetch spec is a WHATWG Web Platform API "Living Standard," has been widely adopted by browsers and has an excellent, stable polyfill for the older browsers we support including IE11. IMO standardized features tend to get the best support and tooling long-term - although jQuery might be an exception to this rule!

Fetch uses promises by default. IMO this makes it compact and expressive, especially when used in combination with newer ES features like async/await.

Fetch rethinks network activity. Some of the pain involved in switching to fetch happens because it comes with some paradigm shifts. For example, it encourages you to think about CORS up front. It also provides Request and Response interfaces to enable standardized caching or mocking of network activity. MDN gives a pretty good overview of this.

Fetch isn't jQuery. This is a personal preference. jQuery is an oddball in our build (still included via ruby gem in production but via yarn in tests) and we've moved away from jQuery as an idiom in general. $.ajax is one of the last things we're using all over the place. Long-term, this may have implications for build time / artifact size.

That's not to say it's perfect - in fact, we should probably be wrapping fetch with our own, nicer interface or a different helper library. Here are a couple of couterarguments: Why I won’t be using Fetch API in my apps, Why I still use XHR instead of the Fetch API.

[islemaster]

What's this for?

[joshlory]

I'm following the pattern in commit 914ee28. When I did this in Game Lab, I was getting random karma:integration test failures when the pending promise would reject after the test finished, when another test was running.

[davidsbailey]

I remember Brent doing some work to deal with state leaking across test runs... but that's all I remember :-/ I'll see if I can dig up any alternate solution.

please correct me if I'm wrong, but it seems like the downside here is that a legit error inside the call to loadLibraries_ would not be caught by this test. on the plus side... the tests are passing, and still succeed in testing everything outside of this block.

[islemaster]

Oh man, debugging integration test issues... I'd like to say any tests hitting this code should stub these requests, but in practice that's not going to happen anytime soon. This seems like a reasonable bit of tech debt to trade for getting this feature up and running now. A comment about what failure mode we're avoiding would be nice though.

[islemaster]

Hmm... I know this isn't a change, but why does our initial render have to wait until the helper libraries are loaded, if we don't need them until we run the program?

[joshlory]

I've been looking for a place to run async code, and guarantee it has completed before a user can run the program. Do you know of the best place to do this?

[islemaster]

A couple of solutions come to mind.

1. The actual run-button-click could depend on an asynchronous getLibraries step, in a lazy-load sense, so that the first run would be slow but subsequent runs would be fast. Then add a fire-and-forget preload step so that the first run is fast unless they manage to hit run before the preload finishes. I think this would work seamlessly on a share page.

2. We could actually disable the run button until the preload finishes, so we're doing the initial render immediately but not allowing the student to try running until the resources are available. This might require some special handling on the share page.

[joshlory]

How do we do it for preloading the animation assets today?

[islemaster]

That happens in a fairly p5.play-specific way. Here's an outline - I won't claim this is an ideal setup though. 😁

Gamelab init() dispatches setInitialAnimationList when the project is loaded.

code-dot-org/apps/src/gamelab/GameLab.js

Lines 402 to 405 in 4ba87b8

	const initialAnimationList =
	(config.initialAnimationList && !config.embed && !config.hasContainedLevels) ?
	config.initialAnimationList : this.startAnimations;
	getStore().dispatch(setInitialAnimationList(initialAnimationList));

setInitialAnimationList in the redux module does a bunch of data-cleaning but eventually calls loadAnimationFromSource for each animation:

code-dot-org/apps/src/gamelab/animationListModule.js

Lines 311 to 320 in 4ba87b8

	return dispatch => {
	dispatch({
	type: SET_INITIAL_ANIMATION_LIST,
	animationList: serializedAnimationList
	});
	dispatch(selectAnimation(serializedAnimationList.orderedKeys[0] || ''));
	serializedAnimationList.orderedKeys.forEach(key => {
	dispatch(loadAnimationFromSource(key));
	});
	};

You've probably guessed, loadAnimationFromSource triggers async loads for all the animation assets into memory, and sets loadedFromSource = true for each of them when complete.

Animations loaded from the library or created by the student are similarly persisted in-memory in redux.

Later, when the user clicks "Run..."

onP5Preload is called while p5 is in the preload phase, after the interpreter has started. It increments (and later decrements) a preload counter within p5 that defers setup() (both student-code and our own) until preload steps are complete.

code-dot-org/apps/src/gamelab/GameLab.js

Lines 1233 to 1241 in 4ba87b8

	GameLab.prototype.onP5Preload = function () {
	Promise.all([
	this.preloadAnimations_(this.level.pauseAnimationsByDefault),
	this.runPreloadEventHandler_()
	]).then(() => {
	this.gameLabP5.notifyPreloadPhaseComplete();
	});
	return false;
	};

preloadAnimations_ waits for animations to be loaded into memory and ready to use, then passes those animations to P5 to be loaded into the engine as animations. It returns a promise which resolves once animations are all in memory in the redux store and we've started loading them into P5. However, loading to P5 is also an async process. It has its own internal effect on the P5 preloadCount, so we don't need to track it here.

code-dot-org/apps/src/gamelab/GameLab.js

Lines 1285 to 1305 in 4ba87b8

	GameLab.prototype.preloadAnimations_ = function (pauseAnimationsByDefault) {
	let store = getStore();
	return new Promise(resolve => {
	if (this.areAnimationsReady_()) {
	resolve();
	} else {
	// Watch store changes until all the animations are ready.
	const unsubscribe = store.subscribe(() => {
	if (this.areAnimationsReady_()) {
	unsubscribe();
	resolve();
	}
	});
	}
	}).then(() => {
	// Animations are ready - send them to p5 to be loaded into the engine.
	return this.gameLabP5.preloadAnimations(
	store.getState().animationList,
	pauseAnimationsByDefault);
	});
	};

preloadAnimations_ doesn't actually start the preload, it just checks areAnimationsReady_ which in turn asks the redux store about each animation:

code-dot-org/apps/src/gamelab/GameLab.js

Lines 1313 to 1316 in 4ba87b8

	GameLab.prototype.areAnimationsReady_ = function () {
	const animationList = getStore().getState().animationList;
	return animationList.orderedKeys.every(key => animationList.propsByKey[key].loadedFromSource);
	};

[islemaster]

setTimeout inside a promise chain feels a little like an antipattern. What exactly are we waiting for here? @davidsbailey might be able to suggest an alternative.

[joshlory]

I think I have a better solution — see commit 3d38c67.

[islemaster]

Can this field be extracted to a partial instead of duplicated across the App Lab and Game Lab editors?

[joshlory]

I'm not super worried about this b/c I think we'll likely move this field up higher (into a shared view) in the future. I'd imagine more level types will eventually have Helper Library code.

[joshlory]

@islemaster PTAL!