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

Convert config service to be strict compliant #8561

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Hinton
Copy link
Member

@Hinton Hinton commented Apr 1, 2024

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Explore how much effort it is to convert services to be strict compliant. I believe the config service should preserve the existing behaviour.

The changes are primarily to:

  • Add undefined or ? to non required properties/return values.
  • Replace some usages of null with undefined. (Not strictly required but it makes the handling easier.
  • Add abstract keywords to abstract service.
  • Rename spec to match the implementation.

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@Hinton Hinton requested a review from a team as a code owner April 1, 2024 09:18
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 1, 2024
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 27.14%. Comparing base (d12953f) to head (3802d24).

Files Patch % Lines
...platform/services/config/default-config.service.ts 50.00% 2 Missing ⚠️
.../src/platform/abstractions/config/server-config.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8561   +/-   ##
=======================================
  Coverage   27.14%   27.14%           
=======================================
  Files        2329     2329           
  Lines       67926    67926           
  Branches    12689    12689           
=======================================
  Hits        18438    18438           
  Misses      48095    48095           
  Partials     1393     1393           

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

This comment was marked as off-topic.

Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Overall, the better way to achieve strict-null in state providers is to redefine key definitions such that the return state is always | undefined rather than forcing implementers to specify it.

Comment on lines +20 to +23
abstract getFeatureFlag$<T extends boolean | number | string>(
key: FeatureFlag,
defaultValue?: T,
) => Observable<T>;
): Observable<T | undefined>;
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to remove the optionality on defaultValue and force a real return.

Same with async impl

Copy link
Member Author

Choose a reason for hiding this comment

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

I can default it to false. I tried to avoid changing the actual behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

It would be better to default based on the incoming type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree with this change though. I think we should postpone making this change until we refactor the feature flags to be able to enforce type checking. That will also allow us to provide a reasonable default value like false, 0, "".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants