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

Rewrite File/Analytics.tsx with no Effect #4224

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Nov 18, 2024

So, something seemed off for me in using Effect here, but I wasn't sure what exactly. And started to dig, because the code is trivial, and it's easy to experiment with it.

To justify using some library, it's useful to implement the same with and without it. I tried to remove Effect to see a difference.

With Effect there are None and Some that doesn't explain intention. And I don't know if an error and lack of results are treated the same way is not a mistake, because it looks like a mistake. (Though, returning Eff.Option.none() explicitly can help)
When I read Some/None statements, I need to jump every time to the location where they are defined.
I can't combine multiple statements into one "if errors or no results" to highlight that this is my intention. (Actually, I can, like you did with defaultExpanded)
I need to decipher, that defaultExpanded is true only when there is data. No simple visible statement "when we show data, we show it in an expanded section".

Three major downsides that handled with Effect:

  • I need to create an additional component where data is defined, because typescript doesn't recognize that object has data when I handled all other states
  • I need to duplicate Section wrapper
  • I have to use React.useEffect to log error

In the conclusion, I see that my main pain point is weak None/Some abstraction. I want to work with states that have descriptive names: #4225
Also, I noted above that using an additional component is a disadvantage, but it's not #4226

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 39.23%. Comparing base (f59ff75) to head (363e454).
Report is 7 commits behind head on replace-select.

Files with missing lines Patch % Lines
catalog/app/containers/Bucket/File/Analytics.tsx 0.00% 11 Missing and 2 partials ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           replace-select    #4224      +/-   ##
==================================================
+ Coverage           39.01%   39.23%   +0.22%     
==================================================
  Files                 780      780              
  Lines               35524    35485      -39     
  Branches             5102     5288     +186     
==================================================
+ Hits                13858    13922      +64     
+ Misses              21095    20367     -728     
- Partials              571     1196     +625     
Flag Coverage Δ
api-python 91.09% <ø> (ø)
catalog 14.55% <0.00%> (+0.29%) ⬆️
lambda 87.95% <ø> (ø)

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.


🚨 Try these New Features:

Base automatically changed from replace-select to master November 19, 2024 12:34
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.

2 participants