[p5.js 2.0] WebGL module syntax

[limzykenneth]

@davepagurek Most of the files are converted to use the new syntax but there are a few snagging issues remaining:

• Some of the classes like p5.DataArray seems largely internal, although they do have a public documentation, are they public API? If not they can have the same treatment as the GeometryBuilder class as a standalone class without being attached to p5.
• In p5.Texture.js, the MipmapTexture class have a compatibility issue with the new system. At export time, there is not p5 available for MipmapTexture to be able to extend p5.Texture, we can move the class Texture into the file scope (ie. not in the new library syntax function(p5, fn){...) but the Texture class itself needs a reference to p5 at definition which is only available in the new library syntax function. We also cannot export MipmapTexture from the library syntax function since export need to be at root. I'm not sure what would be the best way to handle this so if you have some ideas that would be great.
• The actual p5.RendererGL class might need p5.Texture to be solved first and I think you were mentioning wanting to refactor this part. I'll defer this one for now as I also have not converted p5.Renderer2D so there's no rush to do it just yet.

[davepagurek]

Sounds good, DataArray can be made fully internal like GeometryBuilder. I think RenderBuffer could be the same too?

I guess technically we can set arbitrary properties on functions, right? If we want something like multiple exports, we could maybe attach the classes to the module function itself, e.g.:

function texture(p5, fn) {
  p5.Texture = texture.Texture;
  p5.MipmapTexture = texture.MipmapTexture;

  // ...
}

texture.Texture = class Texture { /* ... */ };
texture.MipmapTexture = class MipmapTexture extends texture.Texture { /* ... */ };

export default texture;

...but yeah we might need p5.Texture in RendererGL so it may need to be treated differently. How are we planning on exporting a renderer class in the end?

[limzykenneth]

Setting the class definition on the function as a property probably still won't work as it is also effectively defining the class outside of the library syntax. ie.

texture.Texture = class Texture { /* ... */ };

within class Texture {} there may be reference to p5 and/or fn, for example in p5.Texture's constructor, but at the scope where the class is defined, there is no p5 or fn.

Conventional multiple exports will work fine even when there are default export, but the scope is more of an issue here.

For attaching new renderers, the syntax is as follows:

function newRenderer(p5, fn) {
  p5.renderers["new-renderer"] = NewRendererClass;
}

where p5.renderers are built in and additional renderer classes attach to it. To use the new renderer:

function setup(){
  createCanvas(400, 400, "new-renderer"); // Matching the property key used to attach the renderer class
}

However, if there is a need again to have both the class definition available for things like instanceof and the renderer itself have reference to p5 or fn (which is most likely would), then we will have the same problem again. Attaching to p5 is a possible workaround but not my favorite solution.

[davepagurek]

ahh I see. One option could be to go with a dependency injection sort of approach, where classes that need p5 expect to have it injected as their first parameter. Something like, if you define your class like this:

class Texture {
  constructor(p5, arg1, arg2) {
    this.p5 = p5
  }
}

then expose a p5-style create* function that does this:

function texture(p5, fn) {
  fn.createTexture = function(arg1, arg2) {
    return new Texture(p5, arg1, arg2)
  }
}

I think that would let us make classes extend other classes without immediately having the p5 instance, e.g.:

class MipmapTexture extends Texture {
  constructor(p5, arg1, arg2) {
    super(p5, arg1, arg2)
  }
}

// ...

fn.createMipmapTexture = function(arg1, arg2) {
  return new MipmapTexture(p5, arg1, arg2)
}

We'd lose out on some simplicity though, and it would make the new syntax less easy to use.

[limzykenneth]

A solution that seems to work but I'm still not sure I like it is this:

let MipmapTextureBinding;

function texture(p5, fn){
  p5.Texture = class Texture {}
  MipmapTextureBinding = class MipmapTexture extends p5.Texture {}
}

export {
  MipmapTextureBinding as MipmapTexture
};

export default texture;

[davepagurek]

Right, I guess that comes with the implicit understanding that you'll call texture(), which I could see tripping up some people.

[limzykenneth]

Ah no, this is the library syntax file, the texture function is not used by the user but to attach things with p5.registerAddon(texture), ie. it is called by p5 not by the user.

[limzykenneth]

I guess to summarize a bit, there are three possible solutions (all written below with texture as the library syntax function):

1. Attach MipmapTexture to p5

function texture(p5, fn){
  p5.Texture = class Texture { /* ... */ }
  p5.MipmapTexture = class MipmapTexture extends p5.Texture { /* ... */ }
}

export default texture;

2. Have p5.Texture and MipmapTexture both use the createX style factory functions internally

class Texture {
  constructor(p5, arg1, arg2) {
    this.p5 = p5
  }
}

class MipmapTexture extends Texture {
  constructor(p5, arg1, arg2) {
    super(p5, arg1, arg2)
  }
}

function texture(p5, fn){
  fn.createTexture = function(arg1, arg2) {
    return new Texture(p5, arg1, arg2)
  }

  fn.createMipmapTexture = function(arg1, arg2) {
    return new MipmapTexture(p5, arg1, arg2)
  }
}

export default texture;
export { Texture, MipmapTexture }; // For use with `instanceof`

3. Use late export bindings

let MipmapTextureBinding;

function texture(p5, fn){
  p5.Texture = class Texture {}
  MipmapTextureBinding = class MipmapTexture extends p5.Texture {}
}

export {
  MipmapTextureBinding as MipmapTexture
};

export default texture;

---

Each have their tradeoffs,

1. I don't quite like attaching internal class to p5 as it makes it available publicly when it ideally shouldn't be, although it is relatively simple and consistent in terms of syntax.
2. It somewhat follow the factory function convention used in p5.js overall, however it technically also exposes private classes publicly with attaching to fn.
3. It keeps usage of the classes in other files the same including for things like instanceof, however the library syntax function texture must be called by p5.registerAddon before one attempts to access the MipmapTexture export or the value will be undefined.

@davepagurek Not sure if there is still another option but do you have a preference otherwise?

[limzykenneth]

I've come up with an adjusted version of solution 3 that can partially address its problem:

let MipmapTextureBinding = new Proxy(function(){}, {
  construct(){
    console.error('You are trying to access an export value before it is initialized. Please initialize the library file using `p5.registerAddon` before using this export value.');
    return undefined;
  }
});

function texture(p5, fn){
  p5.Texture = class Texture {}
  MipmapTextureBinding = class MipmapTexture extends p5.Texture {}
}

export {
  MipmapTextureBinding as MipmapTexture
};

export default texture;

This instead of creating confusion in case someone try to use the exported undefined MipmapTexture class before it is initialized, it prints an error message in the console suggesting a possible fix.

@davepagurek If you have a moment can you see if you have any preference around these? I can implement it then merge it in before I continue work on the p5.Graphics stuff.

[davepagurek]

Sorry I missed your earlier comment!

I don't have a super strong preference either way, but I think I generally prefer the first two options for how much easier to understand they are -- scope of variables, especially changing or reassignment of those variables, is a pretty confusing part of JavaScript for newcomers, and it feels like that + a fallback proxy adds a lot of cognitive overhead for making a class.

The downside of options 1 and 2 is that they're not really private. I think this is maybe not the worst thing, since we don't document it, and it lets interested advanced users do custom things if they're interested, similar to how a lot of people still access _renderer. With option 2, I guess we have the option of not exposing a create* function for something private, and just passing p5 in as a parameter, which is a tad more verbose but feels like something we'd need to be doing anyway to be able to handle instance mode as well as global mode for anything accessing p5.

[limzykenneth]

I think in this case I would prefer option 1 for consistency sake as it puts it in the same ballpark as p5.RendererGL etc as a current class.