ronantakizawa/codereview-bench huggingface dataset:
Repository: pixijs/pixijs
File: src/app/Application.ts (TypeScript)

Code before:
 * await app.init();
 * // Add the canvas to the DOM
 * document.body.appendChild(app.canvas);
 * ```
 * @namespace app
 */

/**
 * Any plugin that's usable for Application should contain these methods.
 * @example
 * import { ApplicationPlugin } from 'pixi.js';
 * class MyPlugin implements ApplicationPlugin
 * {
 *    static init(options)
 *    {
 *      // do something with options
 *    }
 *    static destroy()
 *    {
 *     // destruction code here
 *    }
 * }
 * @memberof app
 */
export interface ApplicationPlugin
{
    /**
     * Called when Application is constructed, scoped to Application instance.
     * Passes in `options` as the only argument, which are Application init options.
     * @param {object} options - Application options.
     */
    init(options: Partial<ApplicationOptions>): void;
    /** Called when destroying Application, scoped to Application instance. */
    destroy(): void;
}

/**
 * Application options supplied to the applications init method.
 * @memberof app
 */
export interface ApplicationOptions extends AutoDetectOptions, PixiMixins.ApplicationOptions {}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface Application extends PixiMixins.Application {}

/**
 * Convenience class to create a new PixiJS application.
 *
 * This class automatically creates the renderer, ticker and root container.
 * @example
 * import { Application, Sprite } from 'pixi.js';

Review comment (suggestion):
I cleaned it up but then I searched around and found nothing uses this so I set it to `@ignore`. The "Plugin" vs "Mixin" vs "Extension" is a little confusing.

Code after:
 * await app.init();
 *
 * // don't forget to add the canvas to the DOM
 * document.body.appendChild(app.canvas);
 * ```
 * @namespace app
 */

/**
 * Any plugin that's usable for Application should contain these methods.
 * @example
 * import { ApplicationPlugin } from 'pixi.js';
 *
 * const plugin: ApplicationPlugin = {
 *    init: (options: Partial<ApplicationOptions>) =>
 *    {
 *       // handle init here, use app options if needed
 *    },
 *    destroy: () =>
 *    {
 *       // handle destruction code here
 *    }
 * }
 * @memberof app
 * @see {@link app.ApplicationOptions}
 * @ignore
 */
export interface ApplicationPlugin
{
    /**
     * Called when Application is constructed, scoped to Application instance.
     * Passes in `options` as the only argument, which are Application `init()` options.
     * @param {object} options - Application options.
     */
    init(options: Partial<ApplicationOptions>): void;
    /** Called when destroying Application, scoped to Application instance. */
    destroy(): void;
}

/**
 * Application options supplied to the {@link app.Application#init} method.
 * @memberof app
 * @example
 * import { Application } from 'pixi.js';
 *
 * const app = new Application();
 *
 * await app.init({
 *    autoStart: false,
 *    resizeTo: window,
 *    sharedTicker: true,
Repository: pixijs/pixijs
File: src/assets/cache/parsers/cacheTextureArray.ts (TypeScript)

Code before:
import { ExtensionType } from '../../../extensions/Extensions';
import { Texture } from '../../../rendering/renderers/shared/texture/Texture';

import type { CacheParser } from '../CacheParser';

/**
 * Returns an object of textures from an array of textures to be cached
 *
 * This will be added automatically if `pixi.js/assets` is imported
 * @memberof assets
 */
export const cacheTextureArray: CacheParser<Texture[]> = {
    extension: ExtensionType.CacheParser,

    test: (asset: any[]) => Array.isArray(asset) && asset.every((t) => t instanceof Texture),

    getCacheableAssets: (keys: string[], asset: Texture[]) =>
    {
        const out: Record<string, Texture> = {};

        keys.forEach((key: string) =>
        {
            asset.forEach((item: Texture, i: number) =>
            {
                out[key + (i === 0 ? '' : i + 1)] = item;
            });
        });

        return out;
    }
};


Review comment (suggestion):
if we remove all of these notes about importing `pixi.js/assets` we should still mention it somewhere that it may be required if you decide to manage your own imports

Code after:
import { ExtensionType } from '../../../extensions/Extensions';
import { Texture } from '../../../rendering/renderers/shared/texture/Texture';

import type { CacheParser } from '../CacheParser';

/**
 * Returns an object of textures from an array of textures to be cached
 * @memberof assets
 */
export const cacheTextureArray: CacheParser<Texture[]> = {
    extension: ExtensionType.CacheParser,

    test: (asset: any[]) => Array.isArray(asset) && asset.every((t) => t instanceof Texture),

    getCacheableAssets: (keys: string[], asset: Texture[]) =>
    {
        const out: Record<string, Texture> = {};

        keys.forEach((key: string) =>
        {
            asset.forEach((item: Texture, i: number) =>
            {
                out[key + (i === 0 ? '' : i + 1)] = item;
            });
        });

        return out;
    }
};
Repository: pixijs/pixijs
File: src/rendering/batcher/shared/Batcher.ts (TypeScript)

Code before:
     * keeps a reference to the bind group to set when rendering this batch for WebGPU. Will be null if using WebGl.
     */
    public bindGroup: BindGroup;

    public batcher: Batcher;

    public destroy()
    {
        this.textures = null;
        this.gpuBindGroup = null;
        this.bindGroup = null;
        this.batcher = null;
    }
}

export interface BatchableObject
{
    indexStart: number;

    packAttributes: (
        float32View: Float32Array,
        uint32View: Uint32Array,
        index: number,
        textureId: number,
    ) => void;
    packIndex: (indexBuffer: Uint32Array | Uint16Array, index: number, indicesOffset: number) => void;

    texture: Texture;
    blendMode: BLEND_MODES;
    vertexSize: number;
    indexSize: number;

    // stored for efficient updating..
    textureId: number;
    location: number; // location in the buffer
    batcher: Batcher;
    batch: Batch;

    roundPixels: 0 | 1;
}

let BATCH_TICK = 0;

export interface BatcherOptions
{
    vertexSize?: number;
    indexSize?: number;
}

export class Batcher
{

Review comment (suggestion):
Maybe we should make this a specific type and use instead of the `|` signature. There seem to be enough of them to warrant it.

For example:
```
type IndexBufferArray = Uint32Array | Uint16Array;

packIndex: (indexBuffer: IndexBufferArray, index: number, indicesOffset: number) => void;
```
```

Code after:
     * storing references to these bindgroups here is just faster for access!
     * keeps a reference to the bind group to set when rendering this batch for WebGPU. Will be null if using WebGl.
     */
    public bindGroup: BindGroup;

    public batcher: Batcher;

    public destroy()
    {
        this.textures = null;
        this.gpuBindGroup = null;
        this.bindGroup = null;
        this.batcher = null;
    }
}

export interface BatchableObject
{
    indexStart: number;

    packAttributes: (
        float32View: Float32Array,
        uint32View: Uint32Array,
        index: number,
        textureId: number,
    ) => void;
    packIndex: (indexBuffer: IndexBufferArray, index: number, indicesOffset: number) => void;

    texture: Texture;
    blendMode: BLEND_MODES;
    vertexSize: number;
    indexSize: number;

    // stored for efficient updating..
    textureId: number;
    location: number; // location in the buffer
    batcher: Batcher;
    batch: Batch;

    roundPixels: 0 | 1;
}

let BATCH_TICK = 0;

export interface BatcherOptions
{
    vertexSize?: number;
    indexSize?: number;
}

export class Batcher
Repository: pixijs/pixijs
File: scripts/types/fixTypes.ts (TypeScript)

Code before:
/**
 * -----------------------------------------------------------------
 * The file is used to fix various issues with the types
 * - Add reference paths to the mixins
 * - Replace the dist triple slash reference with the webgpu types
 * - Copy over the Shaders.d.ts file to the lib folder for
 *   proper frag,vert,wgsl support
 * -----------------------------------------------------------------
 */

import fs from 'fs';
import glob from 'glob';
import path from 'path';

/**
 * Get every d.ts file in the src folder and add a
 * reference path to it in the lib/index.d.ts file
 *
 * This allows us to define our PixiMixins in the src folder
 * e.g. /// <reference path="./app/ApplicationMixins.d.ts" />
 */
function addMixinReferencePaths()
{
    const globPath = `${path.join(process.cwd(), './src')}/**/*.d.ts`;
    const files = glob.sync(globPath);

    const lines: string[] = [];

    files.forEach((file) =>
    {
        const name = file.split(`${process.cwd()}/src`)[1];

        lines.push(`/// <reference path=".${name}" />`);
    });

    const filePath = path.join(process.cwd(), './lib/index.d.ts');
    const contents = fs.readFileSync(filePath, 'utf8');
    const updatedContents = `${lines.join('\n')}\n${contents}`;

Review comment (suggestion):
Minor future-proof import suggestions.

```suggestion
import * as fs from 'node:fs';
import * as glob from 'glob';
import * as path from 'node:path';
```


Code after:
/**
 * -----------------------------------------------------------------
 * The file is used to fix various issues with the types
 * - Add reference paths to the mixins
 * - Replace the dist triple slash reference with the webgpu types
 * - Copy over the Shaders.d.ts file to the lib folder for
 *   proper frag,vert,wgsl support
 * -----------------------------------------------------------------
 */

import * as glob from 'glob';
import * as fs from 'node:fs';
import * as path from 'node:path';

/**
 * Get every d.ts file in the src folder and add a
 * reference path to it in the lib/index.d.ts file
 *
 * This allows us to define our PixiMixins in the src folder
 * e.g. /// <reference path="./app/ApplicationMixins.d.ts" />
 */
function addMixinReferencePaths()
{
    const globPath = `${path.join(process.cwd(), './src')}/**/*.d.ts`;
    const files = glob.sync(globPath);

    const lines: string[] = [];

    files.forEach((file) =>
    {
        const name = file.split(`${process.cwd()}/src`)[1];

        lines.push(`/// <reference path=".${name}" />`);
    });

    const filePath = path.join(process.cwd(), './lib/index.d.ts');
    const contents = fs.readFileSync(filePath, 'utf8');
    const updatedContents = `${lines.join('\n')}\n${contents}`;
Repository: pixijs/pixijs
File: tests/renderering/textures/Textures.test.ts (TypeScript)

Code before:
        texture = Texture.from(canvasElement);

        expect(Texture.from(canvasElement)).toBe(texture);
        expect(Cache.has(canvasElement)).toBe(true);

        const buffer = new Uint8Array([
            255, 0, 0, 255, 0, 255, 0, 153,
            0, 0, 255, 102, 255, 255, 0, 51
        ]);

        texture = Texture.from({
            resource: buffer,
            width: 2,
            height: 2,
        });

        expect(Texture.from({ resource: buffer, width: 2, height: 2 })).toBe(texture);
        expect(Cache.has(buffer)).toBe(true);
    });

    it('texture.WHITE should have correct alpha mode set', () =>
    {
        expect(Texture.WHITE.source.alphaMode).toBe('premultiply-alpha-on-upload');
    });

    it.only('should call update a texture correctly', () =>
    {
        const texture = new Texture();

        // spy on texture update event
        const spy = jest.fn();

        texture.on('update', spy);

        texture.update();

        expect(spy).toHaveBeenCalled();
    });
});


Review comment (suggestion):
```suggestion
    it('should call update a texture correctly', () =>
```

Code after:
        texture = Texture.from(canvasElement);

        expect(Texture.from(canvasElement)).toBe(texture);
        expect(Cache.has(canvasElement)).toBe(true);

        const buffer = new Uint8Array([
            255, 0, 0, 255, 0, 255, 0, 153,
            0, 0, 255, 102, 255, 255, 0, 51
        ]);

        texture = Texture.from({
            resource: buffer,
            width: 2,
            height: 2,
        });

        expect(Texture.from({ resource: buffer, width: 2, height: 2 })).toBe(texture);
        expect(Cache.has(buffer)).toBe(true);
    });

    it('texture.WHITE should have correct alpha mode set', () =>
    {
        expect(Texture.WHITE.source.alphaMode).toBe('premultiply-alpha-on-upload');
    });

    it('should call update a texture correctly', () =>
    {
        const texture = new Texture();

        // spy on texture update event
        const spy = jest.fn();

        texture.on('update', spy);

        texture.update();

        expect(spy).toHaveBeenCalled();
    });
});
Repository: pixijs/pixijs
File: src/scene/text/AbstractText.ts (TypeScript)

Code before:
    {
        const { text, resolution, style, anchor, roundPixels, ...rest } = options;

        super({
            ...rest
        });

        this._styleClass = styleClass;

        this.text = text ?? '';

        this.style = style;

        this.resolution = resolution ?? null;

        this.allowChildren = false;

        const anchorX = typeof anchor === 'number' ? anchor : anchor?.x;
        const anchorY = typeof anchor === 'number' ? anchor : anchor?.y;

        this._anchor = new ObservablePoint(
            {
                _onUpdate: () =>
                {
                    this.onViewUpdate();
                },
            },
            anchorX ?? 0,
            anchorY ?? 0
        );

        this.roundPixels = roundPixels ?? false;
    }

    /**
     * The anchor sets the origin point of the text.
     * The default is `(0,0)`, this means the text's origin is the top left.
     *
     * Setting the anchor to `(0.5,0.5)` means the text's origin is centered.
     *
     * Setting the anchor to `(1,1)` would mean the text's origin point will be the bottom right corner.
     *
     * If you pass only single parameter, it will set both x and y to the same value as shown in the example below.
     * @example
     * import { Text } from 'pixi.js';
     *
     * const text = new Text('hello world');
     * text.anchor.set(0.5); // This will set the origin to center. (0.5) is same as (0.5, 0.5).
     */
    get anchor(): ObservablePoint
    {

Review comment (suggestion):
might be a tad tidier to use the setter after constructions? 
 
```if(anchor) this.anchor = anchor;```

Code after:
    {
        const { text, resolution, style, anchor, roundPixels, ...rest } = options;

        super({
            ...rest
        });

        this._styleClass = styleClass;

        this.text = text ?? '';

        this.style = style;

        this.resolution = resolution ?? null;

        this.allowChildren = false;

        this._anchor = new ObservablePoint(
            {
                _onUpdate: () =>
                {
                    this.onViewUpdate();
                },
            },
        );

        if (anchor) this.anchor = anchor;
        this.roundPixels = roundPixels ?? false;
    }

    /**
     * The anchor sets the origin point of the text.
     * The default is `(0,0)`, this means the text's origin is the top left.
     *
     * Setting the anchor to `(0.5,0.5)` means the text's origin is centered.
     *
     * Setting the anchor to `(1,1)` would mean the text's origin point will be the bottom right corner.
     *
     * If you pass only single parameter, it will set both x and y to the same value as shown in the example below.
     * @example
     * import { Text } from 'pixi.js';
     *
     * const text = new Text('hello world');
     * text.anchor.set(0.5); // This will set the origin to center. (0.5) is same as (0.5, 0.5).
     */
    get anchor(): ObservablePoint
    {
        return this._anchor;
    }

    set anchor(value: PointData | number)
Repository: pixijs/pixijs
File: packages/assets/src/loader/parsers/WorkerManager.ts (TypeScript)

Code before:
    private readonly workerPool: Worker[];
    private readonly queue: {
        id: string;
        arguments: any[];
        resolve: (...param: any[]) => void;
        reject: (...param: any[]) => void;
    }[];
    private _initialized = false;
    private _createdWorkers = 0;
    private _isImageBitmapSupported?: Promise<boolean>;

    constructor()
    {
        this.workerPool = [];
        this.queue = [];

        this.resolveHash = {};
    }

    public isImageBitmapSupported(): Promise<boolean>
    {
        if (this._isImageBitmapSupported !== undefined) return this._isImageBitmapSupported;

        this._isImageBitmapSupported = new Promise((resolve) =>
        {
            const worker = (new CheckImageBitmapWorker()).worker;

            worker.addEventListener('message', (event: MessageEvent<boolean>) =>
            {
                worker.terminate();
                CheckImageBitmapWorker.revokeObjectURL();
                resolve(event.data);
            });
        });

        return this._isImageBitmapSupported;
    }

    public loadImageBitmap(src: string): Promise<ImageBitmap>
    {
        return this._run('loadImageBitmap', [src]) as Promise<ImageBitmap>;
    }

    private async _initWorkers()
    {
        if (this._initialized) return;

        this._initialized = true;
    }

    private getWorker(): Worker

Review comment (suggestion):
Small suggestion:
```suggestion
            const { worker } = new CheckImageBitmapWorker();
```


Code after:
    private readonly workerPool: Worker[];
    private readonly queue: {
        id: string;
        arguments: any[];
        resolve: (...param: any[]) => void;
        reject: (...param: any[]) => void;
    }[];
    private _initialized = false;
    private _createdWorkers = 0;
    private _isImageBitmapSupported?: Promise<boolean>;

    constructor()
    {
        this.workerPool = [];
        this.queue = [];

        this.resolveHash = {};
    }

    public isImageBitmapSupported(): Promise<boolean>
    {
        if (this._isImageBitmapSupported !== undefined) return this._isImageBitmapSupported;

        this._isImageBitmapSupported = new Promise((resolve) =>
        {
            const { worker } = new CheckImageBitmapWorker();

            worker.addEventListener('message', (event: MessageEvent<boolean>) =>
            {
                worker.terminate();
                CheckImageBitmapWorker.revokeObjectURL();
                resolve(event.data);
            });
        });

        return this._isImageBitmapSupported;
    }

    public loadImageBitmap(src: string): Promise<ImageBitmap>
    {
        return this._run('loadImageBitmap', [src]) as Promise<ImageBitmap>;
    }

    private async _initWorkers()
    {
        if (this._initialized) return;

        this._initialized = true;
    }

    private getWorker(): Worker
Repository: pixijs/pixijs
File: tests/renderering/text/getFontCss.tests.ts (TypeScript)

Code before:
import { Assets } from '../../../src/assets/Assets';
import { HTMLTextStyle } from '../../../src/scene/text-html/HtmlTextStyle';
import { getFontCss } from '../../../src/scene/text-html/utils/getFontCss';
import { basePath } from '../../assets/basePath';
import { getApp } from '../../utils/getApp';

describe('getFontCss', () =>
{
    beforeAll(async () =>
    {
        await getApp();

        await Assets.load(`${basePath}fonts/outfit.woff2`);
    });

    it.only('should load a font and return css string', async () =>
    {
        const fontFamily = ['Outfit'];
        const style = new HTMLTextStyle({
            fontFamily,
        });
        const css = await getFontCss(
            fontFamily,
            style,
            HTMLTextStyle.defaultTextStyle as {fontWeight: string, fontStyle: string}
        );

        expect(css).toContain('@font-face');
        expect(css).toContain('font-family: "Outfit"');
        expect(css).toContain(`url('data:`);
        expect(css).toContain('font-weight: normal');
        expect(css).toContain('font-style: normal');
    });
});



Review comment (suggestion):
```suggestion
    it('should load a font and return css string', async () =>
```


Code after:
import { Assets } from '../../../src/assets/Assets';
import { HTMLTextStyle } from '../../../src/scene/text-html/HtmlTextStyle';
import { getFontCss } from '../../../src/scene/text-html/utils/getFontCss';
import { basePath } from '../../assets/basePath';
import { getApp } from '../../utils/getApp';

describe('getFontCss', () =>
{
    beforeAll(async () =>
    {
        await getApp();

        await Assets.load(`${basePath}fonts/outfit.woff2`);
    });

    it('should load a font and return css string', async () =>
    {
        const fontFamily = ['Outfit'];
        const style = new HTMLTextStyle({
            fontFamily,
        });
        const css = await getFontCss(
            fontFamily,
            style,
            HTMLTextStyle.defaultTextStyle as {fontWeight: string, fontStyle: string}
        );

        expect(css).toContain('@font-face');
        expect(css).toContain('font-family: "Outfit"');
        expect(css).toContain(`url('data:`);
        expect(css).toContain('font-weight: normal');
        expect(css).toContain('font-style: normal');
    });
});
Repository: pixijs/pixijs
File: src/scene/container/Container.ts (TypeScript)

Code before:
        this._filterEffect = null;
        this.effects = null;
        this._position = null;
        this._scale = null;
        this._pivot = null;
        this._skew = null;

        this.emit('destroyed', this);

        this.removeAllListeners();

        const destroyChildren = typeof options === 'boolean' ? options : options?.children;

        if (destroyChildren)
        {
            for (let i = 0; i < oldChildren.length; ++i)
            {
                oldChildren[i].destroy(options);
            }
        }

        if (this.renderGroup)
        {
            this.renderGroup.destroy();
            this.renderGroup = null;
        }
    }
}

Container.mixin(childrenHelperMixin);
Container.mixin(toLocalGlobalMixin);
Container.mixin(onRenderMixin);
Container.mixin(measureMixin);
Container.mixin(effectsMixin);
Container.mixin(findMixin);
Container.mixin(sortMixin);
Container.mixin(cullingMixin);


Review comment (suggestion):
Nit, take or leave
```suggestion
        this.renderGroup?.destroy();
        this.renderGroup = null;
```

Code after:
        this._maskEffect = null;
        this._filterEffect = null;
        this.effects = null;
        this._position = null;
        this._scale = null;
        this._pivot = null;
        this._skew = null;

        this.emit('destroyed', this);

        this.removeAllListeners();

        const destroyChildren = typeof options === 'boolean' ? options : options?.children;

        if (destroyChildren)
        {
            for (let i = 0; i < oldChildren.length; ++i)
            {
                oldChildren[i].destroy(options);
            }
        }

        this.renderGroup?.destroy();
        this.renderGroup = null;
    }
}

Container.mixin(childrenHelperMixin);
Container.mixin(toLocalGlobalMixin);
Container.mixin(onRenderMixin);
Container.mixin(measureMixin);
Container.mixin(effectsMixin);
Container.mixin(findMixin);
Container.mixin(sortMixin);
Container.mixin(cullingMixin);