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

feat(types): generate addEventListener and removeEventListener overloads to component html element type #4909

Merged

Conversation

m-thompson-code
Copy link
Contributor

@m-thompson-code m-thompson-code commented Oct 9, 2023

This PR updates the compiler to generate overloads needed to provide proper types for addEventListener or removeEventListener for Stencil elements.

What is the current behavior?

Currently, when using addEventListener or removeEventListener, types fallback to HTMLElement which results in invalid types where some kind of CustomEvent is likely expected but Event is used instead.

Likewise, when trying to use Stencil elements within an Angular project's template, the events are mistyped.

GitHub Issue Number: #4908

What is the new behavior?

Now addEventListener and removeEventListener overloads are included with each generated component html element type when added to HTMLElementTagNameMap. Event types line up properly with the user-implemented event types.

Does this introduce a breaking change?

  • Yes
  • No

I don't believe that this introduces breaking changes since all types are mismatched as Event currently.

Testing

I've updated the existing unit tests and added unit tests for the added tests in the compiler. I've also checked manually on an existing Stencil and a new Angular project.

Other information

Before:

ts-before angular-before Screenshot 2023-10-08 at 9 55 42 PM

After:

ts-after angular-after Screenshot 2023-10-08 at 9 56 11 PM

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1399 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/mock-doc/serialize-node.ts 36
src/dev-server/server-process.ts 32
src/compiler/build/build-stats.ts 27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts 25
src/compiler/style/test/optimize-css.spec.ts 23
src/testing/puppeteer/puppeteer-element.ts 23
src/compiler/prerender/prerender-main.ts 22
src/runtime/vdom/vdom-render.ts 20
src/runtime/client-hydrate.ts 19
src/screenshot/connector-base.ts 19
src/compiler/config/test/validate-paths.spec.ts 16
src/dev-server/request-handler.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/compiler/transpile/transpile-module.ts 14
src/runtime/vdom/vdom-annotations.ts 14
src/sys/node/node-sys.ts 14
src/compiler/build/build-finish.ts 13
src/compiler/prerender/prerender-queue.ts 13
Our most common errors
Typescript Error Code Count
TS2345 424
TS2322 398
TS18048 310
TS18047 100
TS2722 38
TS2532 34
TS2531 23
TS2454 14
TS2352 13
TS2769 10
TS2790 10
TS2538 8
TS2344 5
TS2416 4
TS2493 3
TS18046 2
TS2684 1
TS2488 1
TS2430 1

Unused exports report

There are 12 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 115 Env
src/compiler/app-core/app-data.ts 117 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 62 satisfies
src/compiler/types/validate-primary-package-output-target.ts 62 Record
src/testing/puppeteer/puppeteer-declarations.ts 485 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Thanks @m-thompson-code!

Can you do me a favor and generate the typings for us in our Karma and E2E tests for me? When we update type generation, that's still a manual step.

To do so (once this branch is pulled down and built locally with npm run build), the following can be run from the root of the repo:

$ npm run test.karma.prod && npm run test.end-to-end

Then simply commit those changes and push them. Thanks!

@m-thompson-code
Copy link
Contributor Author

Thanks @m-thompson-code!

Can you do me a favor and generate the typings for us in our Karma and E2E tests for me? When we update type generation, that's still a manual step.

To do so (once this branch is pulled down and built locally with npm run build), the following can be run from the root of the repo:

$ npm run test.karma.prod && npm run test.end-to-end

Then simply commit those changes and push them. Thanks!

Hello @rwaskiewicz!

Thanks for the quick response, I've merged main and ran npm run test.karma.prod && npm run test.end-to-end. Tests passed locally, so I've added the updated e2e types.

@rwaskiewicz
Copy link
Member

Thanks @m-thompson-code! At a high level, it looks good to me. I'm going to take a closer look at this PR in the coming week.

@rwaskiewicz rwaskiewicz self-assigned this Oct 10, 2023
@m-thompson-code
Copy link
Contributor Author

Hey @rwaskiewicz let me know if there's anything I can do about the failing CI / Lint and Format and CI / Analysis Tests and if I should keep the branch up to date with main.

@rwaskiewicz
Copy link
Member

Hey @m-thompson-code 👋

Feel free to leave the PR as-is for right now. I plan on taking a look at it this afternoon. The team and I are gonna sync on this tomorrow, so we should have some feedback for you tomorrow afternoon

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Hey there!

Thanks for patience - all in all, this looks really great! I found one minor bug, and had a few tiny grammar/JSDoc nitpicks. I'd like to move forward with this PR. To get that going, can you:

  1. Apply the suggestions in the review (or let me know if you think something doesn't make sense)
  2. Update the tests (they'll break as a consequence of fixing the bug I found)
  3. Format the code (npm run prettier)
  4. Regenerate those typings wiht npm run test.karma.prod && npm run test.end-to-end

Let me know if you have any questions - once those are done, this should be good to merge!

};

/**
* Generates map of event names and user user implemented event type(s). Used to avoid having to write individual
Copy link
Member

Choose a reason for hiding this comment

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

Minor, can we remove the extra 'user' here?

Suggested change
* Generates map of event names and user user implemented event type(s). Used to avoid having to write individual
* Generates map of event names and user implemented event type(s). Used to avoid having to write individual

* @param typeImportData locally/imported/globally used type names, which may be used to prevent naming collisions
* @param sourceFilePath the path to the source file being visited
* @param htmlElementEventMapName the name of the component event map type
* @returns map of event names and user user implemented event type(s)
Copy link
Member

Choose a reason for hiding this comment

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

Same here - can we remove the extra 'user' here?

Suggested change
* @returns map of event names and user user implemented event type(s)
* @returns map of event names and user implemented event type(s)

* Generates map of event names and user user implemented event type(s). Used to avoid having to write individual
* event listener method overloads per event type.
*
* @param cmpEvents a collection of the compiler metadata for a each individual `@Event()`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param cmpEvents a collection of the compiler metadata for a each individual `@Event()`
* @param cmpEvents a collection of the compiler metadata for each individual `@Event()`

ReturnType<typeof StencilTypes.updateTypeIdentifierNames>,
Parameters<typeof StencilTypes.updateTypeIdentifierNames>
>;
let getTextDocsSpy: jest.SpyInstance<ReturnType<typeof Util.getTextDocs>, Parameters<typeof Util.getTextDocs>>;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to spy on/mock this method out here? I'm not seeing any direct calls to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll double-check if these spies do anything. If they do I'll leave a comment or remove them

Parameters<typeof StencilTypes.updateTypeIdentifierNames>
>;
let getTextDocsSpy: jest.SpyInstance<ReturnType<typeof Util.getTextDocs>, Parameters<typeof Util.getTextDocs>>;
let toTitleCaseSpy: jest.SpyInstance<
Copy link
Member

Choose a reason for hiding this comment

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

Same with this spy, is this needed for these tests?

htmlElementEventMap: getHtmlElementEventMap(cmpEvents, typeImportData, cmp.sourceFilePath, htmlElementEventMapName),
htmlElementEventListenerProperties: [
` addEventListener<K extends keyof ${htmlElementEventMapName}>(type: K, listener: (this: ${htmlElementName}, ev: ${cmpEventInterface}<${htmlElementEventMapName}[K]>) => any, options?: boolean | AddEventListenerOptions): void;`,
` addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;`,
Copy link
Member

@rwaskiewicz rwaskiewicz Oct 17, 2023

Choose a reason for hiding this comment

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

I ran into an issue testing this against the Ionic Framework. They have a handful of cases where they use addEventListener and removeEventListener with the following type signatures:

addEventListener<K extends keyof DocumentEventMap>(type: K, listener: (this: Document, ev: DocumentEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;
removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => any, options?: boolean | EventListenerOptions): void;

Can you do me a favor and add those? The first of the two is in the suggestion below:

Suggested change
` addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;`,
` addEventListener<K extends keyof DocumentEventMap>(type: K, listener: (this: Document, ev: DocumentEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;`,
` addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;`,

` addEventListener<K extends keyof ${htmlElementEventMapName}>(type: K, listener: (this: ${htmlElementName}, ev: ${cmpEventInterface}<${htmlElementEventMapName}[K]>) => any, options?: boolean | AddEventListenerOptions): void;`,
` addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;`,
` removeEventListener<K extends keyof ${htmlElementEventMapName}>(type: K, listener: (this: ${htmlElementName}, ev: ${cmpEventInterface}<${htmlElementEventMapName}[K]>) => any, options?: boolean | EventListenerOptions): void;`,
` removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;`,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment above, can we add:

Suggested change
` removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;`,
` removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => any, options?: boolean | EventListenerOptions): void;`,
` removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, actually, after re-reviewing the change suggestion, your suggestion is to add

removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => any, options?: boolean | EventListenerOptions): void;

I thought this property and its pairing:

addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;

wouldn't be needed since they would be inherited from the HTMLElement interface: https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts#L10082

I don't mind adding them in since they shouldn't hurt anything, but what were the issues found without these properties?

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind adding them in since they shouldn't hurt anything

Thanks! Changes look good to me!

but what were the issues found without these properties?

I ran into a few type errors for Ionic Framework components that use addEventListener and removeEventListener for KeyboardEvents.

After checking out this branch, I reverted the changes in c3df57ebe0132073663299425e9ae6188df614a0 (as the changes in it resolve the issue), and built it locally by running the following:

npm run clean \
&& npm ci \
&& npm run build \
&& npm pack

I then checked out the Ionic Framework's main branch (which, as of this writing, is at 068d0038602485085face4a443e473bf1d5cc227).
From there, I was able to reproduce this by running the following, starting in the root of the Ionic Framework repo:

cd core \
&& npm i [PATH_TO_STENCIL_TARBALL] \
&& npm run build \
&& npm run build # second build is intentional

After that second build, we get the following errors:

[20:45.6]  @stencil/core
[20:45.7]  [LOCAL DEV] v4.5.0-dev.1697724915.aee7c9e 🌇
[20:47.5]  build, ionic, prod mode, started ...
[20:47.5]  transpile started ...
[20:51.3]  transpile finished in 3.82 s

[ ERROR ]  TypeScript: src/components/picker-internal/picker-internal.tsx:268:7
           No overload matches this call.Overload 1 of 2, '(type: "ionInputModeChange", listener: (this:
           HTMLIonPickerInternalElement, ev: IonPickerInternalCustomEvent<PickerInternalChangeEventDetail>) => any,
           options?: boolean | ... 1 more ... | undefined): void', gave the following error.Argument of type
           '"keypress"' is not assignable to parameter of type '"ionInputModeChange"'.Overload 2 of 2, '(type: string,
           listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions | undefined):
           void', gave the following error.Argument of type '(ev: KeyboardEvent) => void' is not assignable to
           parameter of type 'EventListenerOrEventListenerObject'.Type '(ev: KeyboardEvent) => void' is not assignable
           to type 'EventListener'.Types of parameters 'ev' and 'evt' are incompatible.Type 'Event' is missing the
           following properties from type 'KeyboardEvent': altKey, charCode, code, ctrlKey, and 17 more.

    L267:  } else {
    L268:    el.addEventListener('keypress', this.onKeyPress);
    L269:    this.destroyKeypressListener = () => {

[ ERROR ]  TypeScript: src/components/picker-internal/picker-internal.tsx:270:9
           No overload matches this call.Overload 1 of 2, '(type: "ionInputModeChange", listener: (this:
           HTMLIonPickerInternalElement, ev: IonPickerInternalCustomEvent<PickerInternalChangeEventDetail>) => any,
           options?: boolean | ... 1 more ... | undefined): void', gave the following error.Argument of type
           '"keypress"' is not assignable to parameter of type '"ionInputModeChange"'.Overload 2 of 2, '(type: string,
           listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions | undefined): void',
           gave the following error.Argument of type '(ev: KeyboardEvent) => void' is not assignable to parameter of
           type 'EventListenerOrEventListenerObject'.

    L269:  this.destroyKeypressListener = () => {
    L270:    el.removeEventListener('keypress', this.onKeyPress);
    L271:  };

[ ERROR ]  TypeScript: src/components/popover/utils.ts:441:3
           No overload matches this call.Overload 1 of 2, '(type: keyof HTMLIonPopoverElementEventMap, listener: (this:
           HTMLIonPopoverElement, ev: IonPopoverCustomEvent<void | OverlayEventDetail<any>>) => any, options?: boolean
           | ... 1 more ... | undefined): void', gave the following error.Argument of type '"keydown"' is not
           assignable to parameter of type 'keyof HTMLIonPopoverElementEventMap'.Overload 2 of 2, '(type: string,
           listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions | undefined):
           void', gave the following error.Argument of type '(ev: KeyboardEvent) => Promise<void>' is not assignable to
           parameter of type 'EventListenerOrEventListenerObject'.Type '(ev: KeyboardEvent) => Promise<void>' is not
           assignable to type 'EventListener'.Types of parameters 'ev' and 'evt' are incompatible.Type 'Event' is not
           assignable to type 'KeyboardEvent'.

    L441:    popoverEl.addEventListener('keydown', callback);
    L442:    return () => popoverEl.removeEventListener('keydown', callback);

[ ERROR ]  TypeScript: src/components/popover/utils.ts:442:16
           No overload matches this call.Overload 1 of 2, '(type: keyof HTMLIonPopoverElementEventMap, listener: (this:
           HTMLIonPopoverElement, ev: IonPopoverCustomEvent<void | OverlayEventDetail<any>>) => any, options?: boolean
           | ... 1 more ... | undefined): void', gave the following error.Argument of type '"keydown"' is not
           assignable to parameter of type 'keyof HTMLIonPopoverElementEventMap'.Overload 2 of 2, '(type: string,
           listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions | undefined): void',
           gave the following error.Argument of type '(ev: KeyboardEvent) => Promise<void>' is not assignable to
           parameter of type 'EventListenerOrEventListenerObject'.

    L441:    popoverEl.addEventListener('keydown', callback);
    L442:    return () => popoverEl.removeEventListener('keydown', callback);
    L443:  };

[20:51.3]  build failed in 3.83 s

We can group those errors into two groups of two, since two occur in ion-popover and two occur in ion-picker. I believe they're running into the same issue/limitation of Stencil's generated types (or TypeScript itself?)

Looking at ion-picker:

export const configureKeyboardInteraction = (popoverEl: HTMLIonPopoverElement) => {
	const callback = async (ev: KeyboardEvent): Promise<void> => {
		// implementation elided
	}

	popoverEl.addEventListener('keydown', callback);   // first error
	return () => popoverEl.removeEventListener('keydown', callback); // second error
}

You're correct in that having a generated interface like:

interface HTMLIonPopoverElement extends Components.IonPopover, HTMLStencilElement {  
    addEventListener<K extends keyof HTMLIonPopoverElementEventMap>(type: K, listener: (this: HTMLIonPopoverElement, ev: IonPopoverCustomEvent<HTMLIonPopoverElementEventMap[K]>) => any, options?: boolean | AddEventListenerOptions): void;  
    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;  
    removeEventListener<K extends keyof HTMLIonPopoverElementEventMap>(type: K, listener: (this: HTMLIonPopoverElement, ev: IonPopoverCustomEvent<HTMLIonPopoverElementEventMap[K]>) => any, options?: boolean | EventListenerOptions): void;  
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;  
}

where HTMLIonPopoverElement extends HTMLStencilElement (which includes my suggested addEventListener and removeEventListener) should pick up the interface definitions from HTMLStencilElement. For some reason, that doesn't appear to be the case though. Prior to applying this change (in Stencil v4.5.0), addEventListener and removeEventListener do in fact resolve to the typings defined on HTMLStencilElement . So something's amuck here, but I think adding the interfaces here are fine for our purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is odd, but yeah, adding the additional overloads would do more help than harm, so I think it'd be fair just to add them. I'll add the additional event listeners to the PR and update everything later tonight.

@m-thompson-code
Copy link
Contributor Author

Hey there!

Thanks for patience - all in all, this looks really great! I found one minor bug, and had a few tiny grammar/JSDoc nitpicks. I'd like to move forward with this PR. To get that going, can you:

  1. Apply the suggestions in the review (or let me know if you think something doesn't make sense)
  2. Update the tests (they'll break as a consequence of fixing the bug I found)
  3. Format the code (npm run prettier)
  4. Regenerate those typings wiht npm run test.karma.prod && npm run test.end-to-end

Let me know if you have any questions - once those are done, this should be good to merge!

Reviewing the suggested code changes, everything makes sense. At a glance, they look good to me. I should be able to update everything tonight or tomorrow :)

@m-thompson-code
Copy link
Contributor Author

Hello @rwaskiewicz ,

  1. I've taken your suggestions and applied them. I don't see anything wrong with these additions
  2. Ran the formatter (found some leftover unused code because of it, haha)
  3. I've built and run the e2e tests locally, and they've passed

Let me know if there's anything else I can do~

@m-thompson-code
Copy link
Contributor Author

Hello @rwaskiewicz ,

  1. I've taken your suggestions and applied them. I don't see anything wrong with these additions
  2. Ran the formatter (found some leftover unused code because of it, haha)
  3. I've built and run the e2e tests locally, and they've passed

Let me know if there's anything else I can do~

Actually, I had one question on one of the suggestions https://github.com/ionic-team/stencil/pull/4909/files#r1362979680

@rwaskiewicz
Copy link
Member

@m-thompson-code Thanks for making those changes!

I responded to your question here. I think this is in a good state to merge, but wanted to give you a chance to respond to my response there before I merge this (and close the issue). I'll wait to hear back from you (even if it's just a " 👍 ") before doing so. Thanks again!

@m-thompson-code
Copy link
Contributor Author

Hello @rwaskiewicz

Thanks for the follow-up to my question. I think that adding the additional property overload should be fine. I've added the last change to address all of your suggestions, ran the formatter and e2e tests. Let me know if there's anything else I can do to help :)

@rwaskiewicz
Copy link
Member

Thanks! I'm running this through our CI workflow now. There may be a few ESBuild related failures. If there are, that's nothing to worry about (building Stencil with ESBuild is a separate project we're running on the side, something that's not used for production builds at this time)

@rwaskiewicz
Copy link
Member

@m-thompson-code Ah, my apologies! I forgot about one more type declaration file we need to update. Can you do me a favor and run the following:

npm run test.analysis

and commit the changed type declaration file? Sorry about that! Other than that, this looks good to me!

@m-thompson-code
Copy link
Contributor Author

@m-thompson-code Ah, my apologies! I forgot about one more type declaration file we need to update. Can you do me a favor and run the following:

npm run test.analysis

and commit the changed type declaration file? Sorry about that! Other than that, this looks good to me!

haha no worries. I ran npm run test.analysis and committed the generated declaration file.

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@rwaskiewicz rwaskiewicz added this pull request to the merge queue Oct 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 20, 2023
@rwaskiewicz rwaskiewicz added this pull request to the merge queue Oct 20, 2023
Merged via the queue into ionic-team:main with commit 0249798 Oct 20, 2023
96 checks passed
@rwaskiewicz
Copy link
Member

Thanks again! This has been merged and is slated to go out as a part of Monday's release 🎉

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.

None yet

2 participants