refactor: Switch to generated jsapi types by mattrunyon · Pull Request #1842 · deephaven/web-client-ui

mattrunyon · 2024-02-28T20:53:54Z

Fixes #1770

Here's the list of changes that weren't just updating the type import

ChartUtils

dh.plot.AxisType.Z does not exist any more. Changed to null for Z for now since we don't do any specific checks for Z axis in the template creation
addTickSpacing add a null check to gapBetweenMajorTicks

Console

Changed openObject to take VariableDescriptor instead of VariableDefinition. Removed title from openObject calls since it's not part of the descriptor

MonacoProviders

monacoToLspPosition added a pick to the return type

TableSaver

cast readonly to non-readonly column array for snapshot call

Integer/Decimal/DateTimeColumnFormatter

Add format.formatString null check in isValid

IrisGridUtils

Removed readonly on columnHeaderGroups line 138
Changed DehydratedSort.direction to just string

IrisGrid

Casting api sort direction (just string) to TableUtils SortDirection (enum)
Changed getOperationMap return type to not readonly (JSAPI doesn't mark as readonly)
Removed readonly from CommonTypes OperationMap
Changed CommonTypes UITotalsTableConfig to not extend dh.TotalsTableConfig because of readonly issues

IrisGridContextMenuHandler

Casted a few ofNumber calls to value as number since we mark it unknown
dateFilterActions changed value to type Date and convert to DateWrapper before number filter value
Throw if value is not Date at line ~500

FormatContextMenuUtils

Line 91 add formatString ?? undefined to coerce string | null | undefined to just string | undefined

IrisGridTreeTableModel

Removed formatRowColor from virtualColumns in constructor. It's marked as static in the types
Cast as unknown as DisplayColumn because it's not a real column

IrisGridTableModelTemplate

Added ?? null to line 1318 because TreeTable setViewport returns void. Table setViewport returns TableViewportSubscription

ServerConfigBootstrap

Added cast to getServerConfigValues to make Map constructor happy

AuthConfigBootstrap

Added cast to getAuthConfigValues to make Map constructor happy

ConsolePanel, Console, useObjectFetcher

Changed some VariableDefinition uses to dh.ide.VariableDescriptor & { title?: string }

DashboardLayout

Removed title from hydrateDefault when metadata.id is null (it's redundant as name is the field we want and title is not part of the descriptor)

FileStorage

Changed FileType to just string to match JS API

IrisGridPanelTypes

Added Partial to isLegacyIrisGridPanelMetadata

Added a blank line to logo.css because of stylelint? Might have accidentally bumped stylelint

codecov · 2024-02-28T21:01:21Z

Codecov Report

Attention: Patch coverage is 49.56710% with 233 lines in your changes are missing coverage. Please review.

Project coverage is 46.08%. Comparing base (fe6c779) to head (f14f40b).

Files	Patch %	Lines
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx	4.76%	20 Missing ⚠️
...ckages/iris-grid/src/IrisGridTableModelTemplate.ts	54.76%	19 Missing ⚠️
packages/jsapi-utils/src/TableUtils.ts	67.34%	16 Missing ⚠️
packages/iris-grid/src/EmptyIrisGridModel.ts	0.00%	15 Missing ⚠️
packages/iris-grid/src/IrisGridUtils.ts	62.16%	14 Missing ⚠️
packages/iris-grid/src/IrisGridProxyModel.ts	60.86%	9 Missing ⚠️
packages/iris-grid/src/IrisGridTreeTableModel.ts	0.00%	8 Missing ⚠️
...ode-studio/src/styleguide/MockIrisGridTreeModel.ts	12.50%	7 Missing ⚠️
...rd-core-plugins/src/panels/DropdownFilterPanel.tsx	12.50%	7 Missing ⚠️
packages/iris-grid/src/IrisGridTableModel.ts	65.00%	7 Missing ⚠️
... and 54 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1842   +/-   ##
=======================================
  Coverage   46.07%   46.08%           
=======================================
  Files         635      635           
  Lines       37978    38013   +35     
  Branches     9586     9608   +22     
=======================================
+ Hits        17499    17517   +18     
- Misses      20425    20443   +18     
+ Partials       54       53    -1

Flag	Coverage Δ
unit	`46.08% <49.56%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mofojed

Not done yet but posting some minor feedback so far. Couple of things to change.

Still need to review the rest.

mofojed · 2024-02-29T21:26:24Z

+          const newAuthConfigValues = (await client.getAuthConfigValues()) as [
+            string,
+            string,
+          ][];


This kind of sucks, but I don't have any clever solutions. Would need a type predicate to map the string[][] to [string, string][] by checking each element I guess? isMapConstructorArray or something.
I also assume that GWT can't express this typing, otherwise that would be the best option to fix it in the jsapi-types source (though can have this temporarily).

I did note this in a non-critical improvements ticket deephaven/deephaven-core#5082

Ya we could add a util to actually check it since this is just assuming it's an array of string pairs

mofojed · 2024-02-29T21:40:13Z

        xaxis: this.makeLayoutAxis(dh.plot.AxisType.X, theme),
        yaxis: this.makeLayoutAxis(dh.plot.AxisType.Y, theme),
-        zaxis: this.makeLayoutAxis(dh.plot.AxisType.Z, theme),
+        zaxis: this.makeLayoutAxis(null, theme),


Ah, on Enterprise it did used to have this constant. I guess we still need to set this for theming purposes of 3d plots in plotly-express? Wondering if we can just remove it and makeLayoutAxis doesn't need to take null then.

We use just the X or Y in makeLayoutAxis to add a few params to X or Y axis specifically

That said, w/ the theme changes I think that could be removed entirely and in the defaults here we could instead do something like this for the 1-2 props we add to the X and Y axis. I'd say do that as a followup though to keep this PR just related to types

xaxix: { ...this.makeLayoutAxis(theme), showgrid: true }

mofojed · 2024-02-29T21:58:04Z

                    column,
                    advancedFilterOptions,
-                    sortDirection,
+                    sortDirection as SortDirection,


Guessing GWT can't express Literal types for sort either... rather than casting should we have a method to convert it and throw if it can't be converted? Would be more "correct".

Side note, not sure how this isn't an error currently, since null shouldn't match SortDirection | undefined

Ya I think there were some issues with it/not a high priority. I'll add a typeguard and check for this.

SortDirection is `"ASC" | "DESC" | "REVERSE" | null``, so that's why it doesn't fail

mofojed · 2024-03-01T14:14:40Z

+      if (name == null) {
+        throw new Error('Column header group has no name');
+      }


Can make this kind of check a one-liner:

Suggested change

if (name == null) {

throw new Error('Column header group has no name');

}

assertNotNull(name, 'Column header group has no name');

Possibly we should be verifying the group name isn't an empty string either, but that doesn't need to be this PR.

mofojed · 2024-03-01T14:35:35Z

  }

-  listenerCleanup: RemoverFn | null;
+  listenerCleanup: (() => void) | null;


Thought for future - this pattern of a cleanup function is common enough, we should have a utility type defined for () => void. Referencing ReturnType<DhType.HasEventHandling['addEventListener']> is possible too but gross. () => void is find and concise, but concern is someone making a mistake when using with other types (e.g. here, doing listenerCleanup: () => void | null instead of listenerCleanup: (() => void) | null).

Anyway just some food for thought.

mofojed · 2024-03-01T15:00:09Z

+    return dh.FilterValue.ofNumber(
+      TableUtils.removeCommas(value) as unknown as number
+    );


Gross. Do we have a follow up ticket to improve JS API? I feel like I commented on this at the time on the JS API review as well as ofNumber needing to take string, but unsure why that wasn't added to the Union type.

deephaven/deephaven-core#5082

mattrunyon added 15 commits January 23, 2024 23:59

Change jsapi-types from local to remote version

c4717aa

Mass rename to dh.*

26bdf15

Fix TS errors

f47bcc3

Handle possibly null name and children from ColumnGroup

6e216cf

Fix tests

07b5aa2

Fix build/dev start

c05d6b7

Fix nullable front columns

db9130f

Merge branch 'main' into jsapi-types

3a4eb0a

Fix ts errors after merge

8dd709c

Merge branch 'main' into jsapi-types

06f90ba

Fix errors after merge

3b04600

Merge remote-tracking branch 'upstream/main' into jsapi-types

b2b083c

Undo WidgetDescriptor removal

8bdcbe2

Merge branch 'main' into jsapi-types

9334c40

Bump jsapi-types version

efe8a18

mattrunyon requested a review from mofojed February 28, 2024 20:53

mattrunyon self-assigned this Feb 28, 2024

mofojed requested changes Feb 29, 2024

View reviewed changes

mofojed requested changes Mar 1, 2024

View reviewed changes

mattrunyon added 2 commits March 4, 2024 16:05

Merge remote-tracking branch 'upstream/main' into jsapi-types

8747f35

Address review comments

f14f40b

mattrunyon requested a review from mofojed March 5, 2024 02:42

mofojed approved these changes Mar 5, 2024

View reviewed changes

mattrunyon merged commit aee4047 into deephaven:main Mar 5, 2024

mattrunyon deleted the jsapi-types branch March 5, 2024 20:35

github-actions Bot locked and limited conversation to collaborators Mar 5, 2024

Conversation

mattrunyon commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mattrunyon commented Feb 28, 2024 •

edited

Loading

codecov Bot commented Feb 28, 2024 •

edited

Loading