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

refactor(cc-tile-requests)!: rework properties to avoid impossible states #1057

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HeleneAmouzou
Copy link
Contributor

@HeleneAmouzou HeleneAmouzou commented May 7, 2024

What does this PR do?

  • Refactors the cc-tile-requests component to implement our new state structure,
  • Implements TypeChecking within the cc-tile-requests component and its stories.

How to review?

  • Check the commit (the diff may not be easy to read so you can review the final result locally if you prefer),
  • If you check it locally, you should not get any Typescript error within the component file or the story file,
  • Compare the prod stories to the preview stories.

Copy link
Contributor

github-actions bot commented May 7, 2024

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-tile-requests/state-migration/index.html.

This preview will be deleted once this PR is closed.

Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done, but see mmy comment bellow.

@florian-sanders-cc florian-sanders-cc requested review from hsablonniere and Galimede and removed request for hsablonniere May 13, 2024 14:29
@florian-sanders-cc florian-sanders-cc force-pushed the cc-tile-requests/state-migration branch 2 times, most recently from 42e50e5 to dab6d06 Compare May 20, 2024 09:18
@Galimede
Copy link
Member

Here are some thoughts on the empty state tile-metrics vs tile-requests:

  • First, we have to take into consideration a fundamental thing: tile-metrics has a smart whereas there's none for tile-requests
  • tile-metrics has three cases for emptiness :
    • CPU data is empty
    • MEM data is empty
    • The app is stopped provokes emptiness (Both of CPU and MEM is empty)
    • This check is done by the smart and not the component itself
  • And this reason is why I think we should keep the tile-metrics behavior.
  • Currently, there's no smart for tile-requests, but someday if we do implement one, I'll definitely go this way.
  • Though there's no smart yet so the other way around works too.

Conclusion/TL;DR: In my opinion, there's no correct answer. However, I do like the cc-tile-metrics way as it's the smart responsibility to handle and calculate its state, the component should only apply its state and not calculate it, it's simply not its job.

Maybe, if we had to make a compromise we could do it the other way around first and when we'll have a smart someday, go for this solution. 🤷‍♂️

I'm sorry @florian-sanders-cc and @HeleneAmouzou that's not a clear yes or no. 😅

@florian-sanders-cc
Copy link
Contributor

Sync about empty states

Considering @Galimede's inputs, we have decided that it made more sense to leave the cc-tile-metrics component unchanged and remove the empty state from cc-tile-requests.

The main reason is that the cc-tile-metrics requires some logic to determine whether the state is empty or not and we prefer leaving it to the smart while cc-tile-requests is more straightforward so the logic may remain within the component itself.

Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…ates

BREAKING CHANGE: the properties have changed
- `state`: new property containing the whole state
- `error`: property has been deleted as it is now part of the state
- `data`: property has been deleted as it is now part of the state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants