chore: Enabled eslint rules requiring return types on functions by bmingles · Pull Request #1503 · deephaven/web-client-ui

bmingles · 2023-09-11T20:04:14Z

Enabled @typescript-eslint/explicit-module-boundary-types for all .ts files
Enabled @typescript-eslint/explicit-function-return-type for non-test files with allowExpressions exclusion
Added a bunch of return types
Added test folders to eslint ignore rules
Added util type to TypeUtils OnlyOneProp

Note, the only runtime changes that should be in this PR are 2 if statements in MockChartModel
https://github.com/deephaven/web-client-ui/pull/1503/files#r1322021147

resolves #1413

bmingles · 2023-09-11T20:07:08Z

-    layout.xaxis.title = 'Datestamp';
-    layout.yaxis.title = 'Price';
+
+    if (layout.xaxis) {


These 2 if statements should be the only 2 runtime code changes in this PR

codecov · 2023-09-11T20:13:35Z

Codecov Report

Patch coverage: 38.12% and project coverage change: -0.01% ⚠️

Comparison is base (1283a34) 46.42% compared to head (4afa235) 46.42%.

❗ Current head 4afa235 differs from pull request most recent head d2583dd. Consider uploading reports for the commit d2583dd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
- Coverage   46.42%   46.42%   -0.01%     
==========================================
  Files         558      558              
  Lines       35691    35706      +15     
  Branches     8915     8917       +2     
==========================================
+ Hits        16571    16576       +5     
- Misses      19069    19080      +11     
+ Partials       51       50       -1

Flag	Coverage Δ
unit	`46.42% <38.12%> (-0.01%)`	⬇️

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

Files Changed	Coverage Δ
packages/app-utils/src/components/useConnection.ts	`0.00% <0.00%> (ø)`
packages/app-utils/src/components/usePlugins.ts	`0.00% <0.00%> (ø)`
packages/app-utils/src/components/useUser.ts	`0.00% <0.00%> (ø)`
packages/chart/src/MockChartModel.ts	`0.00% <0.00%> (ø)`
packages/code-studio/src/AppRoot.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/components/DateInput.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/main/AppControlsMenu.tsx	`15.87% <0.00%> (ø)`
packages/code-studio/src/main/AppInit.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/settings/LegalNotice.tsx	`0.00% <0.00%> (ø)`
packages/code-studio/src/settings/SettingsMenu.tsx	`7.93% <0.00%> (ø)`
... and 118 more

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

mattrunyon · 2023-09-11T20:19:13Z

I'm not sure if we want the explicit return on all functions. We previously only had the module boundary rule enabled.

All functions feels too noisy to me and removes some of the good parts of TS inference. Module boundaries being explicit makes it easier to catch/document public API breakages. Any non-module boundaries will only be used within our code and inferred types will all get checked.

bmingles · 2023-09-11T20:23:21Z

I'm not sure if we want the explicit return on all functions. We previously only had the module boundary rule enabled.

All functions feels too noisy to me and removes some of the good parts of TS inference. Module boundaries being explicit makes it easier to catch/document public API breakages. Any non-module boundaries will only be used within our code and inferred types will all get checked.

@mattrunyon I didn't quite enable it for all functions. The allowExpressions exclusion is enabled, which allows type inference for a lot of cases where we might want it. For all of the return types the new rules made me add, I didn't feel like any of them were heavy handed. Most to all of them add clarity to the codebase IMO.

Do you have any specific examples of where the type inference would be better without the new rules?

mattrunyon · 2023-09-11T20:44:13Z

I don't think most of the added types were heavy, but it feels like writing const x: number = 42; instead of letting TS infer that easily which is one of its benefits. At a glance, there were a bunch of just : void or : Promise<void>.

I think the rule was removed from the default rulesets we consumed because it goes against the power of TS inference, but module boundaries for a library help us catch breaking the API on accident.

If we set up api-extractor we could even remove this linter rule and still have confidence we didn't break our external API. There's already a bunch of inferred return types we have to make explicit because they are module boundaries, but the module is not exposed as part of the public API.

bmingles · 2023-09-11T20:50:43Z

To clarify, this shouldn't require adding types to things like const x: number = 42;. It only requires return types on functions. I'm a fan of type inference when writing code, but explicit function return types can make code more readable since you know what it returns just by looking at the function. It also catches a class of errors where you intend to return a particular type but have a code path in the function that doesn't actually do that.

That said, I'm not hard sold on having to enable this rule, but I do want to highlight the benefits I see in doing so.

mattrunyon · 2023-09-11T21:04:16Z

I know you don't have to type variables, it just feels akin to typing simple variables (although slightly different). I think the class of error where you meant to return something is already handled by the TS compiler. Unless you start using casts and other things to lie to the compiler, in which case the explicit types still won't save you.

I mostly just use hover tooltips if I need to know what a function returns when reading things. I see a bit of benefit for readability, but find the TS inference to be nice for all the simpler functions.

deephaven#1413

mofojed

Overall supportive of the change. Being explicit seems safer, in cases where you might accidentally return the wrong thing/change the return type inadvertently. You can still use type inference to hover over the method to figure out what TS thinks it is if you haven't filled it in yet, but they we should be explicit and lock it in.
Some things to clean up.

mofojed · 2023-09-12T13:19:53Z

 * Component that broadcasts a message when mounted. Should be mounted after the user has logged in.
 */
-export function LoginNotifier() {
+export function LoginNotifier(): null {


Kind of weird just having a null return type here - I was going to suggest ReactNode as that includes null, but then that freaks out in AuthBootstrap. Looks like that issue is fixed in TS 5.1: https://devblogs.microsoft.com/typescript/announcing-typescript-5-1-beta/#decoupled-type-checking-between-jsx-elements-and-jsx-tag-types
If we were >=5.1, might consider using ReactNode instead.

It looks like JSX.Element | null is currently the return type declared by React.FC. Maybe that new 5.1 type will address this, but seems like <SomeComponent/> does not work for things like string which is included in ReactNode. For now I think JSX.Element | null will be the most appropriate for component return types.

mofojed · 2023-09-12T13:23:54Z

+const mapStateToProps = (
+  state: RootState
+): Omit<
+  AppMainContainerProps,
+  'match' | 'setActiveTool' | 'updateDashboardData' | 'updateWorkspaceData'
+> => ({


Another way to separate the props is in Linker.tsx. I kind of like the way Linker is done, what do you think? Doesn't need to be changed in this PR.

I'm a fan of this approach. Wish I'd noticed it before. I won't address on this PR, as there were a bunch of them.

mattrunyon · 2023-09-12T14:00:52Z

@mofojed Echoing my previous comments, but I don't see explicitly typing all returns as that beneficial compared to just the module return types. Any of those mistakes where you accidentally return the wrong type or change the type should automatically be caught by the compiler unless the consuming code (in the same file) is not typed properly. This is not the case for the public API where we want to prevent accidental changes, so typing the module boundaries makes sense.

Just my 2 cents

bmingles · 2023-09-12T14:33:57Z

@mofojed @mattrunyon I ran these rules on Enterprise just to see how big the breaking change is going to be. Looks like 278 errors vs 0 errors for the @typescript-eslint/explicit-module-boundary-types change. I think I may skip adding the @typescript-eslint/explicit-function-return-type rule for now to avoid the breaking change. To Matt's point, the original configuration that we lost was likely the boundary type check, so this at least tackles the scope of the original ticket.

deephaven#1413

bmingles commented Sep 11, 2023

View reviewed changes

bmingles requested review from mattrunyon and mofojed September 11, 2023 20:10

bmingles changed the title ~~chore: Enabled eslint rules requiring return types on functions~~ chore!: Enabled eslint rules requiring return types on functions Sep 11, 2023

bmingles added 4 commits September 12, 2023 08:48

Added return types

28621ff

deephaven#1413

Added return types

47aed6a

deephaven#1413

eslint ignore test folders

915f9a4

deephaven#1413

Enabled @typescript-eslint/explicit-function-return-type

4afa235

deephaven#1413

bmingles force-pushed the 1413-eslint-config branch from 677787b to 4afa235 Compare September 12, 2023 13:48

mofojed requested changes Sep 12, 2023

View reviewed changes

Addressed review comments

d2583dd

deephaven#1413

bmingles requested a review from mofojed September 12, 2023 15:05

bmingles changed the title ~~chore!: Enabled eslint rules requiring return types on functions~~ chore: Enabled eslint rules requiring return types on functions Sep 12, 2023

mofojed approved these changes Sep 13, 2023

View reviewed changes

bmingles merged commit be6747c into deephaven:main Sep 13, 2023

bmingles deleted the 1413-eslint-config branch September 13, 2023 13:31

github-actions Bot locked and limited conversation to collaborators Sep 13, 2023

Conversation

bmingles commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmingles Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattrunyon commented Sep 11, 2023

Uh oh!

bmingles commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattrunyon commented Sep 11, 2023

Uh oh!

bmingles commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattrunyon commented Sep 11, 2023

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

mofojed Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

bmingles Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

mofojed Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

bmingles Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattrunyon commented Sep 12, 2023

Uh oh!

bmingles commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmingles commented Sep 11, 2023 •

edited

Loading

codecov Bot commented Sep 11, 2023 •

edited

Loading

bmingles commented Sep 11, 2023 •

edited

Loading

bmingles commented Sep 11, 2023 •

edited

Loading

bmingles commented Sep 12, 2023 •

edited

Loading