Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add view type as a template for IRenderer, Application #8820

Merged
merged 4 commits into from Nov 6, 2022

Conversation

bigtimebuddy
Copy link
Member

Current Issue

Since we are using ICanvas as a generic interface for OffscreenCanvas, HTMLCanvasElement and Node's canvas, we are getting a type error doing something pretty basic:

const app = new Application();
document.body.appendChild(app.view); // ERROR: Argument of type `ICanvas` is not assignable to type 'Node'

Workaround

Current workaround is to cast:

document.body.appendChild(app.view as HTMLCanvasElement);

Fix

Use a template to specify the type of view.

const app = new Application<HTMLCanvasElement>();
document.body.appendChild(app.view);
const renderer = autoDetectRenderer<HTMLCanvasElement>();
document.body.appendChild(renderer.view);

@bigtimebuddy bigtimebuddy added this to the v7.0.3 milestone Nov 3, 2022
@bigtimebuddy bigtimebuddy requested review from SuperSodaSea and a team November 3, 2022 18:21
Copy link
Member

@Julien-Marcou Julien-Marcou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if the Application could automatically type itself to Application<OffscreenCanvas>, Application<HTMLCanvasElement> or Application<NodeCanvasElement> depending on the context, but I'm not sure that's possible.

packages/app/src/Application.ts Outdated Show resolved Hide resolved
packages/core/src/IRenderer.ts Outdated Show resolved Hide resolved
@Julien-Marcou
Copy link
Member

LGTM!

Copy link
Member

@SuperSodaSea SuperSodaSea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Nov 6, 2022
@bigtimebuddy bigtimebuddy merged commit 814094f into dev Nov 6, 2022
@bigtimebuddy bigtimebuddy deleted the fix/icanvas-type branch November 6, 2022 18:11
bodhivb added a commit to bodhivb/pixel-schedule that referenced this pull request Jan 18, 2023
@171h
Copy link

171h commented Feb 20, 2023

good

@171h
Copy link

171h commented Feb 20, 2023

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants