chore: override OnErrorEventHandlerNonNull #1950
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
|
This would break window.onerror as it does receive a string. We should probably limit this string use case to window and not expose it to other interfaces. Perhaps we can modify GlobalEventHandlers.onerror with the general |
Okay |
|
Hello @saschanaz |
|
Hmm. Maybe we should first make some unit tests in https://github.com/microsoft/TypeScript-DOM-lib-generator/tree/main/unittests/files, something like: // onerror.ts
// this should not show error
window.onerror = (message: string, source: string, lineno: number, colno: number, error: any) => {};
// this also should not show error
declare var img: HTMLImageElement;
img.onerror = (event: Event) => {};The goal here would be to make both pass. |
|
I have found some issues that i will fix |
inputfiles/addedTypes.jsonc
Outdated
| }, | ||
| "onerror": { | ||
| "name": "onerror", | ||
| "overrideType": "(event: Event) => void" |
There was a problem hiding this comment.
This would only solve HTMLImageElement, maybe it should go to HTMLElement to solve it for other HTML elements.
There was a problem hiding this comment.
And the CI fails: Error: generated/dom.generated.d.ts(11577,11): error TS2430: Interface 'HTMLImageElement' incorrectly extends interface 'HTMLElement'.
There was a problem hiding this comment.
Hello @saschanaz,
I have applied the changes to all elements.
I was facing a build error due to the lack of documentation on the JSON format. However, replacing GlobalEventHandlers with Omit<GlobalEventHandlers, 'onerror'> solves the issue.
Maybe we could start documenting the format to help others in the future?
There was a problem hiding this comment.
Omit sounds ok, but maybe you didn't push your change?
Documentations sounds nice. I wonder we can have a local schema file.
There was a problem hiding this comment.
Maybe just adding some comments to existing type declarations would be okay.
There was a problem hiding this comment.
Hello @saschanaz
My question is how to override the type?
I have tried to add it to the overriding types but it didn't work.
|
Hello @saschanaz |
fixes #1821