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 Event Types #165

Merged
merged 2 commits into from
Jan 31, 2025
Merged

Add Event Types #165

merged 2 commits into from
Jan 31, 2025

Conversation

greggman
Copy link
Contributor

this makes addEventListener on GPUDevice have the right type information.

Screenshot 2025-01-29 at 11 50 18

Screenshot 2025-01-29 at 11 51 12

I wasn't sure how to add it to the generated file.

@greggman greggman requested a review from kainino0x January 29, 2025 19:55
@greggman
Copy link
Contributor Author

maybe these should go in the spec so they're upstream?

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

maybe these should go in the spec so they're upstream?

There is unfortunately no way to describe these in WebIDL (except for attribute EventHandler onuncapturederror;).

I actually thought we had this already, but I guess not!

dist/main.d.ts Outdated
@@ -0,0 +1,16 @@
import './index.d.ts';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Historically we've been just adding things like this to the top of index.d.ts:

types/dist/index.d.ts

Lines 5 to 49 in 8faeb6d

// *********************************************************************************************
// Manually-written
// *********************************************************************************************
interface HTMLCanvasElement {
getContext(
contextId:
| "webgpu"
): GPUCanvasContext | null;
}
interface OffscreenCanvas {
getContext(
contextId:
| "webgpu"
): GPUCanvasContext | null;
}
// Defined as an empty interface here to prevent errors when using these types in a worker.
interface HTMLVideoElement {}
type GPUOrigin2DStrict =
| Iterable<GPUIntegerCoordinate>
| GPUOrigin2DDictStrict;
interface GPUOrigin2DDictStrict
extends GPUOrigin2DDict {
/** @deprecated Does not exist for GPUOrigin2D. */
z?: undefined;
}
type GPUExtent3DStrict =
| Iterable<GPUIntegerCoordinate>
| GPUExtent3DDictStrict;
// GPUExtent3DDictStrict is defined to help developers catch a common class of errors.
// This interface defines depth as an undefined, which will cause a type check failure if someone
// attempts to set depth rather than depthOrArrayLayers on a GPUExtent3D (an easy mistake to make.)
interface GPUExtent3DDictStrict
extends GPUExtent3DDict {
/** @deprecated The correct name is `depthOrArrayLayers`. */
depth?: undefined;
}

It's probably fine to have multiple files, but we should have all of the additions in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved what I could.

dist/main.d.ts Outdated
@@ -0,0 +1,16 @@
import './index.d.ts';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I don't think we should have "index" and "main" as those both denote main files. We could move the existing index.d.ts to another filename though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it to types.d.ts

@greggman
Copy link
Contributor Author

greggman commented Jan 31, 2025

note: I separated this into 2 commits. If you look at the commit diffs instead of the PR diff it's easier to see the changes.

@greggman
Copy link
Contributor Author

sorry, I'll fix the tests

This makes addEventListener on GPUDevice have the right type
information.
@greggman
Copy link
Contributor Author

ugh. Ok. I just added the types to the original dist/index.d.ts

I could not figure out how to separate it into 2 files.

The issue was with index.d.ts (new file with manual types) importing types.d.ts (the IDL generated file). It was getting errors about types it didn't know. See https://github.com/gpuweb/types/actions/runs/13063705687/job/36452157799#step:5:18

So, I imported those with

import {
  GPUCanvasConfigure,
  GPUIntegerCoordinate',
  ...
} "./types.d.ts";

This shut-up TS about undefined types but now it complained types.d.ts is not a module (some voodoo magic ts stuff). If I make it a module (needs to export something) then it would no longer add its types to the gloal since it's now a module and not some type file. To make it export its types requires wrapping them all in declare global { ... } but if I do that then prettier is going to reformat the entire file with 2 space indents and that means the diff between generated/index.d.ts and dist/types.d.ts would be every line. I could add some code to generate generated/index.d.ts with those lines and that formatting but it was getting ridiculous. My goal was to add ~7 lines of code. Not to spend hours making TS happy.

So, if we want those separate files we can do that in another PR. For now I just added the changes to dist/index.d.ts

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM

dist/index.d.ts Outdated Show resolved Hide resolved
@kainino0x
Copy link
Collaborator

Thanks for looking into splitting it into two files. It's not super important, so if we can't easily figure out how to do it then it's fine if we don't. It also matters less until we have the codegen working better (assuming we ever bother to fix it).

@greggman greggman enabled auto-merge (rebase) January 31, 2025 02:25
@greggman greggman merged commit 860e557 into gpuweb:main Jan 31, 2025
1 check passed
@greggman greggman deleted the add-event-types branch January 31, 2025 05:08
@kainino0x
Copy link
Collaborator

Just remembered to publish: 0.1.54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants