Poolable TypeScript refactoring

[jonathanolson]

I've finally got Poolable in a state where it can work with seemingly all of the constraints. I'll collect commits here and ask for a review.

[jonathanolson]

@zepumph would you be able to review or discuss? If it looks good, I'd like to convert more things over (since this was the blocking pattern for Vector2).

Also, proper TS of Action and Emitter seem like the most helpful thing next.

[zepumph]

Also, proper TS of Action and Emitter seem like the most helpful thing next.

I think we should really take into account https://github.com/phetsims/phet-io/issues/1543. We were going to totally refactor those two classes, but I haven't had time to do it.

[zepumph]

I don't like how we have to typecast the options arg. I don't think we should need to do this, but I can't understand what's different about this usage of optionize that it doesn't think that defaultArguments has been filled in. My best guess is the as unknown as ConstructorParameters<Type>. But I made a wilder example (8), and it didn't fail just because it was an generic, even when I added my own as unknown as MyGeneric<T> to it.

[zepumph]

I also tried this, but it didn't really work.

defaultArguments: ( class TempClass {} ).prototype.constructor.arguments,

[zepumph]

Maybe you can get farther than I did with my example, but I was running into problems trying to duplicate the issue in wilder:

Index: js/wilder/model/WilderOptionsPatterns.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/wilder/model/WilderOptionsPatterns.ts b/js/wilder/model/WilderOptionsPatterns.ts
--- a/js/wilder/model/WilderOptionsPatterns.ts	(revision 2c0da78c87c3bd78b4556135e05f3ed10ec136c8)
+++ b/js/wilder/model/WilderOptionsPatterns.ts	(date 1644440768593)
@@ -59,6 +59,7 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
+import Constructor from '../../../../phet-core/js/Constructor.js';
 import optionize from '../../../../phet-core/js/optionize.js';
 import { Defaults } from '../../../../phet-core/js/optionize.js';
 import wilder from '../../wilder.js';
@@ -318,18 +319,22 @@
 
 type WrapTypeOptions<T> = {
   favoriteGeneric?: MyGeneric<T>
+
+  defaultArguments?: ConstructorParameters<T>
 };
 
 
-class WrapType<T> {
+class WrapType<T extends Constructor> {
   favoriteGeneric: MyGeneric<T>;
 
   constructor( providedOptions?: WrapTypeOptions<T> ) {
     const options = optionize<WrapTypeOptions<T>, WrapTypeOptions<T>>( {
-      favoriteGeneric: new MyGeneric<T>()
+      favoriteGeneric: new MyGeneric<T>(),
+      defaultArguments: [] as unknown as ConstructorParameters<T>
     }, providedOptions );
 
     this.favoriteGeneric = options.favoriteGeneric;
+    this.x = options.defaultArguments;
   }
 
   getFavoriteItemProperty(): MyGeneric<T> {
@@ -339,10 +344,10 @@
 
 console.log( new WrapType() );
 console.log( new WrapType( {
-  favoriteGeneric: new MyGeneric<boolean>( true )
+  favoriteGeneric: new MyGeneric<MyItem>( new MyItem() )
 } ) );
 console.log( new WrapType( {
-  favoriteGeneric: new MyGeneric<boolean>()
+  favoriteGeneric: new MyGeneric<MyItem>()
 } ) );
 
 ////////////////////////////////////////////////////////////////////////////////////////////////

[pixelzoom]

This is blocking progress on Function Builder redeploy in phetsims/function-builder#139.

[zepumph]

@jonathanolson ping me if you would like to pair. I'm happy to spend some time here.

[zepumph]

@jonathanolson and I were able to discuss a new Poolable pattern on Monday. @jonathanolson committed it and I believe is feeling pretty good about it. @jonathanolson, does this still block publication? Can this issue be closed?

[jonathanolson]

Added Pool.ts above. It gets rid of the mixin handling for pooling. This seems like the best pattern, and the IDE can detect createFromPool parameter names. It's not too much boilerplate and seems to simplify things. It also works just fine with abstract supertypes. See SVGLinearGradient for an example usage, it's basically:

1. Optionally add implements IPoolable
2. Add the equivalent of:

  freeToPool() {
    SVGLinearGradient.pool.freeToPool( this );
  }

  static pool = new Pool( SVGLinearGradient );

where options can be added to the Pool constructor as needed.

Then it's SVGLinearGradient.pool.createFromPool( ... )

Thoughts @zepumph?

[zepumph]

I wrote this line at the end of SVGLinearGradient:

SVGLinearGradient.pool.createFromPool();

And it errored with:

TS2554: Expected 2 arguments, but got 0.
. . . 
Pool<typeof SVGLinearGradient>.createFromPool(     svgBlock: SVGBlock,     gradient: Gradient): SVGLinearGradient

Really great stuff! I love it. I would argue that this is less boilerplate than the javascript mixin. Anything else for this issue?

[jonathanolson]

I believe the commit above handles most issues brought up in #103 (comment).

Also, this line is very exciting:

Can I take that as a compliment?

Also, it seems unclear which parts of this implementation are done in a more complex way in order to obtain better performance characteristics.

Most of the effort in the typing is to make sure we get proper types for pool methods, to prevent errors in use of pooled types. The JS parts are mostly just in creating objects and partly performance. Mostly for usability.

[jonathanolson]

Why does TPoolable only promote freeToPool? and Why freeToPool instead of any of the other public Pool functions?

You don't want to have to know an object's concrete type in order to free it to its pool. A motivating example is in RichText, where you could have children of various different types of poolable objects (RichTextLeaf, RichTextElement, RichTextLink). You want to be able to call .freeToPool() on any of them, and have them go to their separate pools.

Since the object's only action within a pool is to free it TO that pool, it doesn't promote other things. Modifying things about pools without knowing what pool you are dealing with seems... inadvisable.

Does it make sense to not implement TPoolable and instead just include a static pool method on any classes we want to provide pool functionality to?

Would that mean removing object.freeToPool() as noted above? That would require tracking of what type each object is, and that doesn't seem great.

[marlitas]

Review continued...

• A motivating example is in RichText, where you could have children of various different types of poolable objects (RichTextLeaf, RichTextElement, RichTextLink). You want to be able to call .freeToPool() on any of them, and have them go to their separate pools.

  • I dove into RichText pool implementations and found that there is still method forwarding that I don't see is adding additional type information.

 public freeToPool(): void {
    RichTextLeaf.pool.freeToPool( this );
  }

  public static readonly pool = new Pool( RichTextLeaf );

It seems that all usages of freeToPool() are forwarding the method call to the class' static pool function, and the type of the object is tracked by the pool's constructor.

public constructor( type: T, providedOptions?: PoolableOptions<T, Params> ) {
const options = optionize<PoolableOptions<T, Params>, PoolableOptions<T, Params>>()( {

      defaultArguments: [] as unknown as Params,
      initialize: ( type.prototype as unknown as { initialize: PoolableInitializer<T, Params> } ).initialize,
      maxSize: 100,
      initialSize: 0,
      useDefaultConstruction: false
    }, providedOptions );

...
this.partialConstructor = Function.prototype.bind.bind( type, type );

I still don't see how freeToPool() is adding additional type information that new Pool( object ) is not. Perhaps there's some other aspect of forwarding code that I'm missing? If it's better to have a synchronous conversation about this let me know.

• I'm concerned about ( type.prototype as unknown as { initialize: PoolableInitializer<T, Params> } ).initialize
  • Unless the initialize option is overwritten, we are assuming that the type passed into the constructor will have an initialize function. This seems like another critical property for an object to indeed be "poolabe". The type casting hides the fact that the passed in type may not have an initialize function which I feel looses type safety in doing so.
  • Additionally there is no clarity that if the object does not have an initialize function the Pool option should be overwritten.

[jonathanolson]

I dove into RichText pool implementations and found that there is still method forwarding that I don't see is adding additional type information.

I agree it doesn't add type information. I'm a bit confused what you mean here.

[jonathanolson]

Unless the initialize option is overwritten, we are assuming that the type passed into the constructor will have an initialize function.

The type casting hides the fact that the passed in type may not have an initialize function which I feel looses type safety in doing so.

That is very true. It seems insufficiently documented.

It seems possible (but very difficult, probably at least 8 tricky typescript lines and a ts-ignore because optionize doesn't handle this pattern) to type this nicely. The trick is "if you don't provide an initialize option, you HAVE to provide a type with initialize, OTHERWISE there is no requirement".

I'm prone to improved documentation and assertions, instead of adding a lot of type system complexity to handle that case.

Additionally there is no clarity that if the object does not have an initialize function the Pool option should be overwritten.

So, would more documentation help?

[marlitas]

I'm prone to improved documentation and assertions, instead of adding a lot of type system complexity to handle that case.

So, would more documentation help?

Yes. @jonathanolson I think improved documentation would help since it does seem like trying to handle this through typescript would become overly complex. I also wonder if adding an assertion could be helpful?

I agree it doesn't add type information. I'm a bit confused what you mean here.
JO and I discussed over Slack meeting up. I think that will clarify our confusion here, as I am also confused.

[marlitas]

We are closing this as completed and opening a new issue to convert PDOMinstance.ts to using the new Pool phetsims/scenery#1543, as well as js => ts conversion tracked in these issues:

• TypeScript conversion kite#100
• Drawables typescript conversion scenery#1544

[phet-dev]

Reopening because there is a TODO marked for this issue.

[zepumph]

https://github.com/phetsims/dot/blob/2307e5e7eb009a08f142c38b929207a648b1ffc9/js/Bounds2.ts#L35-L37