Skip to content

fix: setting default never for data$ #936

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

Merged
merged 5 commits into from
Jun 17, 2021
Merged

fix: setting default never for data$ #936

merged 5 commits into from
Jun 17, 2021

Conversation

anandtiwary
Copy link
Contributor

@anandtiwary anandtiwary commented Jun 17, 2021

Description

Please include a summary of the change, motivation and context.
fix: setting default never for data$

This will allow us to set dynamic loading message

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #936 (f3a2b23) into main (fa25c51) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #936   +/-   ##
=======================================
  Coverage   85.14%   85.14%           
=======================================
  Files         806      806           
  Lines       16643    16644    +1     
  Branches     2097     2098    +1     
=======================================
+ Hits        14170    14171    +1     
  Misses       2441     2441           
  Partials       32       32           
Impacted Files Coverage Δ
...ojects/components/src/spinner/spinner.component.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa25c51...f3a2b23. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@anandtiwary anandtiwary marked this pull request as ready for review June 17, 2021 01:25
@anandtiwary anandtiwary requested a review from a team as a code owner June 17, 2021 01:25
@@ -52,9 +52,13 @@ export class SpinnerComponent implements OnChanges {

public state$?: Observable<SpinnerAsyncState>;

public ngOnInit(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

onInit shouldn't be necessary on top of the onChanges. In fact, these days I find onInit is rarely what we want, it just distract from real workhorse, onChanges. I'd consider it maybe not a red flag, but a yellow one.

Specifically in this case as long as data$ is assigned, line 61 would have already run before we got here. This would only be useful in the case where data$ is never assigned (which isn't really a use case, but if you want to default state$ to NEVER on line 53, that's fine, avoids making it optional too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't assign anything to data then like 61 wouldn't execute. Bare minimum, i will have to assign undefined.

In my use case, I am not assigning anything to data$. So Spinner will always be in loading state and on the parent side we will just keep updating the loading message like 'Received 10 events' -> Received 20 events

Copy link
Contributor

Choose a reason for hiding this comment

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

You could just assign NEVER and it would work for your special case, right? why build it into the component?

Copy link
Contributor Author

@anandtiwary anandtiwary Jun 17, 2021

Choose a reason for hiding this comment

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

I could. But doesn't it read better? Also, shouldn't the spinner support undefined/missing data$ case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way, doesn't feel particularly better to me. That said, if you prefer that my initial suggestion was to default it to NEVER anyway - just via assignment rather than onInit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, let me confirm it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep or just public state$: Observable<SpinnerAsyncState> = of(SpinnerAsyncState.Loading);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Don't you have more code reviews in another 7 hours?

Copy link
Contributor

Choose a reason for hiding this comment

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

more like 5

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@anandtiwary anandtiwary merged commit 5de8ca7 into main Jun 17, 2021
@anandtiwary anandtiwary deleted the spinner-fix branch June 17, 2021 05:15
@github-actions
Copy link

Unit Test Results

    4 files  ±0  258 suites  ±0   14m 25s ⏱️ - 2m 37s
929 tests ±0  929 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
935 runs  ±0  935 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 5de8ca7. ± Comparison against base commit fa25c51.

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