refactor(tests): reduce TypeScript errors by maxpatiiuk · Pull Request #10344 · Esri/calcite-design-system

maxpatiiuk · 2024-09-18T21:44:26Z

Run the migration script on tests.
This PR includes some small fixups that affect TypeScript typings to reduce number of TypeScript errors after the codemod.
With these changes, there are only 57 TypeScript error in the entire calcite-components package after the codemod!

maxpatiiuk · 2024-09-18T21:44:46Z

      expect(chipSelectSpy2).toHaveReceivedEventTimes(0);

-      await chip5.setAttribute("selected", true);
+      await chip5.toggleAttribute("selected", true);


This is a boolean attribute
Before it was being set here to selected="true"

maxpatiiuk · 2024-09-18T21:46:27Z

    let comboboxItem: E2EElement;
    let itemNestedLi: E2EElement;
-    let closeEvent: Promise<void>;
+    let closeEvent: Promise<unknown>;


Stencil's waitForEvent has return type Promise<any> so this worked.
In Lumina, the return type is Promise<SerializedEvent>.

(at runtime, Stencil's waitForEvent also returned Promise, but they didn't publicly expose the SerializedEvent type)

maxpatiiuk · 2024-09-18T21:49:05Z

   */
  @Event({ cancelable: false })
-  calciteInternalListItemGroupDefaultSlotChange: EventEmitter<DragEvent>;
+  calciteInternalListItemGroupDefaultSlotChange: EventEmitter<void>;


it didn't actually emit this type, and no one was listening for it to emit this type

maxpatiiuk · 2024-09-18T21:51:25Z

-        aria-valuemax={isDeterminate ? 100 : undefined}
-        aria-valuemin={isDeterminate ? 0 : undefined}
-        aria-valuenow={isDeterminate ? valueNow : undefined}
+        aria-valuemax={isDeterminate ? "100" : undefined}


Stencil's aria-valuemax type is string | number, and at runtime, the DOM stringifies numbers automatically, so this was good.
However, the TypeScript's type for HTMLElement.ariaValuemax is string. Since this code is converted by the codemod to this.el.ariaValuemax = ..., we need to be setting strings here to avoid type error.

maxpatiiuk · 2024-09-18T21:51:53Z

      async function cycleThroughItemsAndAssertValue(keys: "left-right" | "up-down"): Promise<void> {
        const [moveBeforeArrowKey, moveAfterArrowKey] =
-          keys === "left-right" ? ["ArrowLeft", "ArrowRight"] : ["ArrowUp", "ArrowDown"];
+          keys === "left-right" ? (["ArrowLeft", "ArrowRight"] as const) : (["ArrowUp", "ArrowDown"] as const);


Lumina's typings for await element.press(moveAfterArrowKey); are more strict

maxpatiiuk · 2024-09-18T21:52:14Z

-    const rowPos = event["detail"].rowPosition;
-    const destination = event["detail"].destination;
-    const lastCell = event["detail"].lastCell;
+  calciteInternalTableRowFocusEvent(event: CustomEvent<TableRowFocusEvent>): void {


looks like a mistake in typing.
TableRowFocusEvent is the payload type, not event type

maxpatiiuk · 2024-09-18T21:53:22Z


    const element = await page.find("calcite-text-area");
-    element.setAttribute("max-length", 5);
+    element.setAttribute("max-length", "5");


Stencil's E2EElement.setAttribute had type (string, any)=>void
In Lumina, it is (string, string)=>void so that it matches the HTMLElement.setAttribute type.
I want to align us closer with DOM typings so that migrating to Vitest browser mode is easier

maxpatiiuk · 2024-09-18T21:53:47Z

   * @internal
   */
-  @Event({ cancelable: false }) calciteInternalTimePickerChange: EventEmitter<string>;
+  @Event({ cancelable: false }) calciteInternalTimePickerChange: EventEmitter<void>;


never actually included a string payload type, and no one was listening for it

maxpatiiuk · 2024-09-18T21:54:29Z

    let dispatchTransitionEvent: TransitionEventDispatcher;
-    let onStartCallback: jest.Mock<any, any, any>;
-    let onEndCallback: jest.Mock<any, any, any>;
+    let onStartCallback: jest.Mock;


With this change, the type would be equivalent both in Jest and Vite.
Otherwise it was causing issues because Vite's Mock has only 2 type arguments

jcfranco

Other than a few comments, this is looking great! Thanks, @maxpatiiuk!

jcfranco · 2024-09-19T00:32:26Z

  @Listen("calciteInternalListItemGroupDefaultSlotChange")
  @Listen("calciteInternalListDefaultSlotChange")
-  handleCalciteInternalListDefaultSlotChanges(event: CustomEvent<void>): void {
+  handleCalciteInternalListDefaultSlotChanges(event: CustomEvent): void {


Do Lumina's type set CustomEvent.detail's' type as void by default?

yes!

To be more precise, regarding authoring events:

default payload type in createEvent() is undefined, rather than any like it was in Stencil.

it is a typescript error to not provide a payload to the .emit() function if the event has non-undefined payload type

the codemod will update EventEmitter<void> to EventEmitter as needed

edit:
Regarding listening to events, CustomEvent.detail is still any since that's coming from TypeScript.
I also see that you used CustomEvent<void> in many places, so I will revert this place to use CustomEvent<void> for consistency

jcfranco · 2024-09-19T00:37:09Z

 }

-export interface InteractiveContainerProps extends JSXAttributes {
+export interface InteractiveContainerProps extends JSXAttributes<HTMLDivElement> {


This could stay the same as we're not using any props from a particular HTML element.

Applies to XButtonProps too.

Without the change in XButtonProps, I was getting a type error here:

calcite-design-system/packages/calcite-components/src/components/functional/XButton.tsx

Line 31 in 0b03a57

ref={ref}

The issue is that in Lumina JSX, the ref prop can be assigned either (element: HTMLButtonElement) => void or { value: HTMLButtonElement } (like a RefObject type in React).

The RefObject type is causing issues.
By default JSXAttributes interface is assuming HTMLElement type.
That means the ref prop type in the <XButton> is RefObject<HTMLElement>, which is not assignable to the RefObject<HTMLButtonElement> in the <button>.

The same issue is present in React since they also have object refs. Not an issue in Stencil since they don't have object refs, only callback refs.

The change in <InteractiveContainer> is not necessary, but I done it for consistency, and to avoid issues if someone does add a ref prop to it in the future.

I could be misunderstanding, but it seems this change is conflating the prop types with the ref type. These can be different depending on the implementation, right?

It doesn't seem like we're using XButton's ref property, so we could alternatively drop it for the time being.

I could be misunderstanding, but it seems this change is conflating the prop types with the ref type

JSXAttributes represents the common props type, including the ref prop.
The generic parameter you pass to JSXAttributes (eg JSXAttributes<HTMLDivElement>) dictates the type used in the ref

It doesn't seem like we're using XButton's ref property, so we could alternatively drop it for the time being.

Sounds good, I will go ahead and remove it.

You're right. I was thinking about other use cases where we might want to provide a ref to another part of the functional component, but this would probably be better off as a separate property in the props interface.

maxpatiiuk · 2024-09-19T15:34:51Z

Looks like GitHub action was stuck on E2E tests for 6 hours and then timed out:

https://github.com/Esri/calcite-design-system/actions/runs/10933000573/job/30350737756

Triggering E2E re-run completed successfully 🤔

maxpatiiuk · 2024-09-24T19:16:17Z

          }

-          return orig.call(input, init);
+          return orig.call(window, input, init);


First argument to .call is this.
Not sure why this worked before in Stencil, but this causes issues after migration.

jcfranco

👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤
👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘🤘👨‍🎤👨‍🎤🤘🤘🤘👨‍🎤👨‍🎤🤘🤘🤘👨‍🎤🤘🤘🤘🤘👨‍🎤🤘👨‍🎤
👨‍🎤🤘🤘👨‍🎤👨‍🎤🤘👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤👨‍🎤🤘👨‍🎤
👨‍🎤🤘👨‍🎤🤘👨‍🎤🤘👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤👨‍🎤🤘🤘🤘👨‍🎤👨‍🎤🤘👨‍🎤
👨‍🎤🤘👨‍🎤👨‍🎤🤘🤘👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤
👨‍🎤🤘👨‍🎤👨‍🎤👨‍🎤🤘👨‍🎤👨‍🎤🤘🤘👨‍🎤👨‍🎤🤘🤘🤘👨‍🎤👨‍🎤🤘🤘🤘👨‍🎤🤘🤘🤘🤘👨‍🎤🤘👨‍🎤
👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤👨‍🎤

…tracking * origin/dev: (40 commits) feat: add parcel parameter (#10384) feat(alert): apply --calcite-alert-corner-radius to internal close button (#10388) feat(dialog, panel): Add css properties for background-color (#10387) fix: remove aria-disabled from components where necessary (#10374) feat(action-group, block, panel): add `menuPlacement` and `menuFlipPlacements` properties (#10249) fix(list, filter): fix sync between list items and filtered data (#10342) feat(popover): apply component tokens to arrow (#10386) feat(list-item): add `unavailable` property (#10377) fix(segmented-control): honor appearance, layout and scale when items are added after initialization (#10368) fix(action-pad): fix horizontal action group alignment (#10359) fix(combobox): selection-mode="single-persist" correctly selects an item when items have same values (#10366) fix(input-time-zone): fix region mode labeling and value mapping (#10345) fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264) refactor(components): reduce post-migration TypeScript errors (#10356) refactor: do not use Host in functional components (#10352) refactor(tests): reduce TypeScript errors (#10344) feat(alert): add component tokens (#10218) fix(card): properly handle slotted elements (#10378) refactor(panel): remove duplicate tailwind class (#10379) feat(popover, action): add component tokens (#10253) ...

…estone-estimates * origin/dev: (29 commits) fix(input-time-zone): fix region mode labeling and value mapping (#10345) fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264) refactor(components): reduce post-migration TypeScript errors (#10356) refactor: do not use Host in functional components (#10352) refactor(tests): reduce TypeScript errors (#10344) feat(alert): add component tokens (#10218) fix(card): properly handle slotted elements (#10378) refactor(panel): remove duplicate tailwind class (#10379) feat(popover, action): add component tokens (#10253) chore(t9n): add translations (#10339) feat: add 3d building, 3d building parameter, divide, parcel, spaces, spaces parameter, square brackets x, n variable, zoning parameter (#10373) build: update browserslist db (#10370) ci: resolve husky pre-push and pre-commit errors (#10365) docs: update component READMEs (#10371) feat(text-area): add component tokens (#10343) fix(action): prefer `disabled` in favor of `aria-disabled` (#10367) docs(a11y): add reference to a11y guidance to issue template (#10372) chore(action): add new string for accessible indicator label (#10360) feat(chip): add component tokens (#10179) feat(avatar): add component tokens (#10280) ...

maxpatiiuk self-assigned this Sep 18, 2024

github-actions Bot added the refactor Issues tied to code that needs to be significantly reworked. label Sep 18, 2024

maxpatiiuk commented Sep 18, 2024

View reviewed changes

jcfranco approved these changes Sep 19, 2024

View reviewed changes

maxpatiiuk added 3 commits September 24, 2024 12:00

refactor(tests): reduce TypeScript errors

2ba7034

revert: undo loosening CustomEvent type

9316229

refactor(functional): remove unused props

6e7d00a

maxpatiiuk force-pushed the max/tests-typing branch 2 times, most recently from f430d28 to 6e7d00a Compare September 24, 2024 19:05

fix(tests): correctly call native fetch

cf7ffb3

maxpatiiuk commented Sep 24, 2024

View reviewed changes

maxpatiiuk requested a review from jcfranco September 24, 2024 20:43

jcfranco approved these changes Sep 24, 2024

View reviewed changes

jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 24, 2024

jcfranco merged commit 2240d80 into dev Sep 24, 2024

jcfranco deleted the max/tests-typing branch September 24, 2024 23:03

github-actions Bot added this to the 2024-09-30 - 2.13.0 Sep Main release milestone Sep 24, 2024

jcfranco mentioned this pull request Sep 24, 2024

Migrate component framework to Lit #10310

Closed

Conversation

maxpatiiuk commented Sep 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxpatiiuk Sep 18, 2024 • edited by jcfranco Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxpatiiuk Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxpatiiuk commented Sep 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxpatiiuk Sep 18, 2024 •

edited by jcfranco

Loading

maxpatiiuk Sep 19, 2024 •

edited

Loading