quiltdata / quilt

Quilt is a data mesh for connecting people with actionable data
https://quiltdata.com
Apache License 2.0
1.33k stars 90 forks source link

Rewrite File/Analytics.tsx with enum #4225

Closed fiskus closed 3 days ago

fiskus commented 5 days ago

So, I didn't like to work with Some/None, and rewrote the code with enum, enumerating states explicitly. It turns out I don't need Effect for this.

Cons:

Pros:

Using Typescript tuples makes sure data exists when State.Data.

Also, if we use an additional component, we can get rid of self-invoking function, but duplicate <Section />

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 9.89583% with 346 lines in your changes missing coverage. Please review.

Project coverage is 39.21%. Comparing base (2039532) to head (279fab0). Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...talog/app/containers/Bucket/Overview/Downloads.tsx 25.37% 93 Missing and 7 partials :warning:
catalog/app/containers/Bucket/Overview/Header.tsx 0.00% 71 Missing :warning:
...atalog/app/containers/Bucket/Overview/Overview.tsx 0.00% 62 Missing and 1 partial :warning:
catalog/app/containers/Bucket/File/Analytics.tsx 0.00% 41 Missing and 2 partials :warning:
catalog/app/embed/File.js 0.00% 21 Missing and 1 partial :warning:
catalog/app/utils/AWS/S3.js 0.00% 9 Missing and 3 partials :warning:
catalog/app/containers/Bucket/File/File.js 0.00% 11 Missing :warning:
...atalog/app/containers/Bucket/Overview/ColorPool.ts 0.00% 7 Missing and 1 partial :warning:
.../app/containers/Bucket/requests/requestsUntyped.js 14.28% 6 Missing :warning:
...rs/Bucket/File/gql/ObjectAccessCounts.generated.ts 0.00% 2 Missing :warning:
... and 6 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4225 +/- ## ========================================== + Coverage 39.02% 39.21% +0.19% ========================================== Files 772 780 +8 Lines 35540 35499 -41 Branches 5311 5297 -14 ========================================== + Hits 13870 13922 +52 + Misses 21063 20381 -682 - Partials 607 1196 +589 ``` | [Flag](https://app.codecov.io/gh/quiltdata/quilt/pull/4225/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | Coverage Δ | | |---|---|---| | [api-python](https://app.codecov.io/gh/quiltdata/quilt/pull/4225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `91.09% <ø> (ø)` | | | [catalog](https://app.codecov.io/gh/quiltdata/quilt/pull/4225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `14.54% <9.89%> (+0.24%)` | :arrow_up: | | [lambda](https://app.codecov.io/gh/quiltdata/quilt/pull/4225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `87.95% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

nl0 commented 5 days ago

this kind of thing is more commonly done with disjoint unions rather than with plain TS enums, e.g.

interface Loading {
  _tag: 'Loading'
}
interface NoAnalytics {
  _tag: 'NoAnalytics'
}
interface Data {
  _tag: 'Data'
  data: ...
}
type State = Loading | NoAnalytics | Data

that way you keep the tag and the data in one place. there are helpers for this in effect:

import { Data } from "effect"

// Define a union type using TaggedEnum
type State = Data.TaggedEnum<{
  Loading: {}
  NoData: {}
  Data: { readonly data: ... }
}>

also, for such a general case, i'd use an abstraction like RemoteData or smth.

oh, and having different states for data / no data seems excessive -- Data: Option<...> expresses this notion just fine imo.

fiskus commented 5 days ago

having different states for data / no data

It's not about if the request has data or not. It's about whether to show data or not. That's type for UI, View-data. That particular component shows "No analytics" when the request is failed. That looks like a mistake. But if this is intentional, we can have comment like "it's legit to have error, that means we don't have analytics" or some named variable.

fiskus commented 3 days ago

Was created for demonstration only