pixi.js-base • pixi.js-bunny-mark

npm i https://pkg.pr.new/pixijs/pixijs/pixi.js@11710

commit: a95f31b

Hi!
Thank you for your contribution!

I have some thoughts:

• BlurFilterPassOptions are explicitly waiting for horizontal to be defined (it's not optional)
  • Which means that horizontal will always be defined and will never be implicitly inferred from undefined or null. So there is no need to coerce it from defaultOptions, nor declare it there.

  export interface BlurFilterPassOptions extends BlurFilterOptions
  {
      /** Do pass along the x-axis (`true`) or y-axis (`false`). */
      horizontal: boolean;
  }
  
  this.blurXFilter = new BlurFilterPass({ horizontal: true, ...options });
  this.blurYFilter = new BlurFilterPass({ horizontal: false, ...options });

• Changes in the BlurFilter constructor's signature give no benefit. They are practically equal and wrong semantically: resolution explicitly defines that it takes null as value, which is treated differently than undefined.
• There is no need to define const _options, since we are already creating a fresh new object and copying its content from the options argument, which guarantees consistency, even if values in the original options will change.

   options = { ...BlurFilterPass.defaultOptions, ...options };

• So, taking into account everything written above, MakeRequired in this context seems artificial, and I'm not sure about the motivation.

The goal in #8852 is to get the typescript to be pass with the strictNullChecks = true improving type safety. The changes are in service of that. Set strictNullChecks = true it in tsconfig.json and look at the errors reported when running npm run test:types.

@mayakwd Read over your feedback and made some changes.

• BlurFilterPassOptions : Fixed.
• Changes in the BlurFilter constructor: I was trying to update the types so that the overloads matched but I made a mistake that I have corrected.
• const _options change was made to tell typescript that _options is a different type from options. options is the constructor input and _options is the defaults merged with options so there are properties that are no longer possibly undefined. At build time I believe this would be merged into one variable. In ts you cannot replace the type of a variable after it is initialized. So you do const _options = { ...BlurFilter.defaultOptions, ...options } which will infer a different type than options.
• I added FilterDefaultOptions, BlurFilterDefaultOptions, and BlurFilterPassDefaultOptions instead of using MakeRequired. This is cleaner and less brittle.

Default Options Pattern Problem

There is an issue with the public static defaultOptions pattern used when trying to be more strict with ts. Currently Filter.defaultOptions is typed to FilterOptions which is an object with all its properties optional. All properties that are optional in FilterOptions need to be required in Filter.defaultOptions to ensure fallback values. So I updated the type to Filter.defaultOptions: Required<FilterOptions> .

This revealed that static properties in extended classes have to also extend the type in ts. So to be valid it needs to be like this:

class export class BlurFilter extends Filter 
{
  public static defaultOptions: BlurFilterDefaultOptions = {
    ...Filter.defaultOptions,
    strength: 8,
    quality: 4,
    kernelSize: 5,
  };
}

So the BlurFilter.defaultOptions type needs to extend the Filter.defaultOptions. The constructor code/inheritance in pixijs is written in a way that this seems like it should still work but it would be double merging parent default values in child class constructors and then again in the parent. I haven't been able to find a work around for this. Maybe someone else can.

Currently the code is written so that (in most cases) constructor options arg and public static defaultOptions are (incorrectly) the same type so they are compatible in child classes.

Conclusions

Writing ts with strictNullChecks = true makes code less elegant and more verbose. But it does increase type safety and catch potential bugs. I've already found some in other areas while working on this. Pixijs was not written with these strict rules/patterns in mind and may not be wise to attempt to adhere to them.

If the #8852 chore is no longer desired then it should be closed. Maybe other areas can be improved but leave the defaultOptions pattern as is?