add before/after preload and setup

[capGoblin]

Resolves #6422

Changes:

Here I have added the missing beforePreload, afterPreload, beforeSetup and afterSetup. @davepagurek would like to have your review on this implementation or anything wrong with this logic. Waiting for your feedback.

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[davepagurek]

Thanks for taking this on! The control flow of preload and setup is a little complicated because preload kicks off some asynchronous functions, so I think we want our after preload hook to get run after those complete (just to confirm, @msawired is that the behaviour that would be the most useful?)

I've added some suggestions of where to put these functions to make that happen, let me know if I can explain anything more!

[davepagurek]

I think we need to move this one into the _runIfPreloadsAreDone method. The preload() call might start loading things over the network, and that method will get called when all of those have finished.

[davepagurek]

I think this branch only gets called of the sketch doesn't have a preload function, otherwise _runIfPreloadsAreDone will trigger setup to be called. To make sure we call these regardless of what triggers _setup(), maybe we can add these calls inside of _setup itself?

[capGoblin]

Hmm, that makes sense. I've refactored the code, and I'd love to do any other changes.

[msawired]

... I think we want our after preload hook to get run after those complete (just to confirm, @msawired is that the behaviour that would be the most useful?)
@davepagurek Yes, that is correct 👍

[davepagurek]

It looks like this happens after _setup() on line 299. Maybe we can move this to be before this if statement instead of after?

[davepagurek]

We should probably loop through and call everything in target._registeredMethods[key] for each of these

[msawired]

With the method above, how do we register a method to, say, afterSetup?
Would this work?
p5.on("afterSetup", function(){...})

[davepagurek]

Pretty close! It should be p5.prototype.registerMethod('afterSetup', function() { ... }) (and unregisterMethod to remove it later.)

[capGoblin]

I have added callRegisteredHooksFor to call all the registered before/after hooks for preload/setup. Also calling the afterPreload before the _setup() which i missed.

[davepagurek]

Nice work on this helper function! Just for consistency, now that we have this helper, can we update the pre/post draw hooks to use this too? Currently that happens here:

p5.js/src/core/structure.js

Line 486 in 63f94b0

context._registeredMethods.pre.forEach(callMethod);

I also notice that for pre/post draw, rather than passing this to call, they pass context, where it is defined as:

p5.js/src/core/structure.js

Line 469 in 63f94b0

const context = this._isGlobal ? window : this;

Maybe we can do that here too?

[davepagurek]

I think the last thing we should try doing is writing a unit test to confirm that these get called when expected. Is that something you've tried before in p5? I can help point you to the right files as examples if you need

[davepagurek]

Ah, looks like tests are failing because this is undefined. You might need to make this a method on p5.prototype and then call this.callRegisteredHooksFor(...) as a method instead of a function?

[capGoblin]

Thanks for your help!

[capGoblin]

of course I'd love to do that. I haven't had the chance to write unit tests in p5 before, so any guidance or examples you can provide would be really helpful.

[davepagurek]

It looks like there's one test so far for registerMethod here:

p5.js/test/unit/core/main.js

Lines 5 to 33 in 63f94b0

	test('should register and call "init" methods', function() {
	var originalInit = p5.prototype._registeredMethods.init;
	var myp5, myInitCalled;
	p5.prototype._registeredMethods.init = [];
	try {
	p5.prototype.registerMethod('init', function myInit() {
	assert(
	!myInitCalled,
	'myInit should only be called once during test suite'
	);
	myInitCalled = true;
	this.myInitCalled = true;
	});
	myp5 = new p5(function(sketch) {
	assert(sketch.hasOwnProperty('myInitCalled'));
	assert(sketch.myInitCalled);
	sketch.sketchFunctionCalled = true;
	});
	assert(myp5.sketchFunctionCalled);
	} finally {
	p5.prototype._registeredMethods.init = originalInit;
	}
	});

You'd probably be adding a very similar test right after this one. I think the idea would be to make a boolean to record whether each lifecycle callback has been called yet, and register methods setting each corresponding boolean to true. Then, in your sketch, at each point in the lifecycle, you can assert that the boolean values are what you expect. e.g. for setup, it could look something like this:

return new Promise((resolve) => {
  let afterSetupCalled = false
  p5.prototype.registerMethod('afterSetup', () => {
    afterSetupCalled = true
  })
  myp5 = new p5(function(sketch) {
    sketch.setup = () => {
      assert.equal(afterSetupCalled, false)
    }
    sketch.draw = () => {
      assert.equal(afterSetupCalled, true)
      resolve()
    }
  })
 })

...but extended to test all the lifecycle methods. You can resolve the test promise when we've waited for the last lifecycle event (which might still be in draw, but may need to be in the second frame draw to make sure that the post hook works after the first frame's draw?)

[capGoblin]

@davepagurek I have added the tests definitely need review on that. And also do i have to backup the original _registeredMethods in a var like we did originalInit and put that back finally if so why?

[davepagurek]

Right, we also need to reset the callbacks. The reason for it is that we add them to p5.prototype, which is like the class that all p5 instances inherit from. We make a new p5 instance in each test, but since we're essentially adding the callbacks to the class they all share, they would build up.

A maybe quicker way to do it (rather than pasting it into each test) would be to add a teardown function inside the suite that all these tests are in, which will get run after each test. Something like:

suite('p5.prototype.registerMethod', function() {
  teardown(function() {
    p5.prototype._registeredMethods.init = [];
    // Clear other methods here too. It's probably fine to just reset them to
    // empty arrays rather than saving and restoring an initial value.
  });
  test('should register and call before and after "preload" hooks', function() {
    // ...
  });
});

[davepagurek]

Currently I think this will resolve before setup or preload gets run. Does it work to move the resolve into sketch.setup after your assertion?

[davepagurek]

Getting a postDraw assertion is going to be a bit tough. The reason why we can't do assertions outside of setup and draw here is that those actually get run first, before draw is called: when we define draw, we're only defining it, not calling it, and it gets called asynchronously later after preload and setup finish. So if we resolve outside of it, then likely we exit before draw gets called at all.

As an alternative: the first time draw gets called, sketch.frameCount will be 1. So we could do something like this, to make sure it gets run after the first draw call:

sketch.draw = () => {
  if (sketch.frameCount === 2) {
    assert.equal(postDrawCalled, true);
    resolve();
  }
};

[capGoblin]

I've implemented the recommended changes and also cleaned the init test a bit. Thanks for your clear explanations and it all makes sense now!

[davepagurek]

Looks good, thanks @capGoblin!

[davepagurek]

Oh actually one last thing before we merge would be to update the .md file here again with the new methods: https://github.com/processing/p5.js/blob/main/contributor_docs/creating_libraries.md Then we're good to merge!

[davepagurek]

Great work @capGoblin!