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(compiler): generate component custom event types with HTML target #3296

Merged
merged 17 commits into from
May 9, 2022

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Mar 21, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

The generated types for a web components custom event, does not include type definitions for the target value.

GitHub Issue Number: #3291

What is the new behavior?

  • Stencil compiler will generate new exported interfaces in components.d.ts for all web component that have custom events.
  • The generated interfaces includes the typing to the generated HTML element type for the event target and accepts a generic for the custom event detail.
  • Stencil compiler will generate the web component custom event type with the new type, allowing usage with the custom event and the generated HTML element type for the target.

Does this introduce a breaking change?

  • Yes
  • No

Testing

The steps outlined below can apply to any Stencil project, but will outline against Ionic Framework:

  1. Build and pack a local version of this branch
  2. Install the .tgz into Ionic Framework /core project.
  3. Run npm run build to build Stencil
  4. See that in components.d.ts that there is a new namespace called ComponentEvents.
  5. Build the react package, by running npm run build from packages/react
  6. Sync the build changes with the react test app, by running npm run sync from packages/react/test-app.
  7. Run the test-app npm run start
  8. Modify any of these .tsx files (i.e. App.tsx) to use a web component with a JSX event listener that accesses a custom public method on the web component:
<IonInfiniteScroll onIonInfinite={ev => ev.target.complete()}></IonInfiniteScroll>
  1. Validate that there is no typing errors/exceptions. Intellisense/tsc is able to determine the type of the complete() function from infinite-scroll.tsx.

Other information

Closing #3286 in favor of this PR.

@sean-perkins
Copy link
Contributor Author

I can open a PR to the Stencil docs site to account for these changes, if we agree with the approach and naming of the generated types.

@sean-perkins
Copy link
Contributor Author

sean-perkins commented Mar 21, 2022

I could see us renaming the generated types to be: ComponentEvents.IonAccordion instead of ComponentEvents.IonAccordionCustomEvent (as an example). But will wait until this gets a first look.

@sean-perkins
Copy link
Contributor Author

Discussed with the framework team, only concession is if we could rather export all the generated event interfaces globally, instead of under a namespace.

This would make implementations similar to:

import { InfiniteScrollCustomEvent } from '@ionic/core';

onInfiniteScroll(ev: InfiniteScrollCustomEvent) {
  ev.target.complete();
}

I originally opted not to do this, based on the type definition file having everything namespaced or under a module. If we so no risk to exporting the interfaces all top-level, I'd like to make that change.

@rwaskiewicz
Copy link
Member

rwaskiewicz commented Apr 22, 2022

@sean-perkins We landed #2936 #3337, which was blocking us from moving forward on this PR. LMK if you wanna sync up some time next week to figure out next steps for this one

@sean-perkins
Copy link
Contributor Author

@rwaskiewicz Sounds good! I'll rebase and incorporate some of the changes we discussed offline (probably ~ mid next week). I'll reach out if I have questions with the latest 👍

@sean-perkins
Copy link
Contributor Author

This PR has been updated. Unsure if you can re-run failed jobs or if I need to push an empty commit to re-trigger CI. I know we can re-run failed actions in framework, assume it is a contributor permission.

@rwaskiewicz
Copy link
Member

yeah...must be a contributor permission thing. kicked it back off

this commit adds a single unit test for the new
`generate-event-detail-types` file. this was created to ensure that we
codify the fact that the interface generated by this file follows a
specific format
@@ -6,20 +6,28 @@ import { updateTypeIdentifierNames } from './stencil-types';
* Generates the individual event types for all @Event() decorated events in a component
* @param cmpMeta component runtime metadata for a single component
* @param typeImportData locally/imported/globally used type names, which may be used to prevent naming collisions
* @param cmpClassName The pascal cas name of the component class
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 cmpClassName The pascal cas name of the component class
* @param cmpClassName The pascal cased name of the component class

@@ -32,6 +40,7 @@ export const generateEventTypes = (cmpMeta: d.ComponentCompilerMeta, typeImportD
*/
const getEventType = (
cmpEvent: d.ComponentCompilerEvent,
cmpEventDetailInterface: string,
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 add the JSDoc for this new parameter?

@rwaskiewicz rwaskiewicz merged commit 846740f into ionic-team:main May 9, 2022
rwaskiewicz added a commit that referenced this pull request May 12, 2022
this commit fixes type checker errors associated with
`StencilTypes.updateTypeIdentifierNames` that were missed in the
original implementation.

these errors were found while reviewing #3296 by happen-stance.
STENCIL-445 has been created to investigate how we can run the type
checker on tests more often & catch these errors sooner.
rwaskiewicz added a commit that referenced this pull request May 12, 2022
this commit fixes type checker errors associated with
`StencilTypes.updateTypeIdentifierNames` that were missed in the
original implementation.

these errors were found while reviewing #3296 by happen-stance.
STENCIL-445 has been created to investigate how we can run the type
checker on tests more often & catch these errors sooner.
rwaskiewicz added a commit that referenced this pull request May 18, 2022
this commit checks in changes to `test/end-to-end/src/components.d.ts`
that are generated starting with 846740f
(#3296)

we missed this as a result of relying on ci to run the end to end tests
for us, rather than ever running them locally. the changes found in this
commit were generated by running `npm run test.end-to-end` on the commit
immediately preceding this one.

to avoid issues like this in the future, stencil-451 has been created
and added to the backlog
rwaskiewicz added a commit that referenced this pull request May 19, 2022
this commit checks in changes to `test/end-to-end/src/components.d.ts`
that are generated starting with 846740f
(#3296)

we missed this as a result of relying on ci to run the end to end tests
for us, rather than ever running them locally. the changes found in this
commit were generated by running `npm run test.end-to-end` on the commit
immediately preceding this one.

to avoid issues like this in the future, stencil-451 has been created
and added to the backlog
@moshner
Copy link

moshner commented May 31, 2022

This is great! Thanks for adding it. Are there docs on how to use it?

@rwaskiewicz
Copy link
Member

@moshner There aren't any explicit docs. However, if you upgrade to Stencil v2.16.0 and re-compile your project, these types should be autogenerated for you

@moshner
Copy link

moshner commented May 31, 2022

Docs would be very helpful. Especially, for anyone who is consuming our web components and need to hook into events.

@jcfranco
Copy link
Contributor

jcfranco commented Jun 1, 2022

This is a great enhancement! Is there a particular reason why currentTarget isn't typed? Apologies if I'm missing something.

rwaskiewicz added a commit that referenced this pull request Sep 20, 2022
this commit updates the typings for the todo-app that exists within the
`test/` subdirectory of the project. these changes should have been
committed with 846740f (#3296) but were unfortunately missed
rwaskiewicz added a commit that referenced this pull request Sep 21, 2022
this commit updates the typings for the todo-app that exists within the
`test/` subdirectory of the project. these changes should have been
committed with 846740f (#3296) but were unfortunately missed
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.

4 participants