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

Start using @lifeomic/react-client #531

Merged
merged 5 commits into from
Feb 21, 2024
Merged

Start using @lifeomic/react-client #531

merged 5 commits into from
Feb 21, 2024

Conversation

nicolls1
Copy link
Contributor

@nicolls1 nicolls1 commented Feb 14, 2024

Changes

  • Start using @lifeomic/react-client

Screenshots

@nicolls1 nicolls1 changed the title first round of updates refactor: start using @lifeomic/react-client Feb 14, 2024
@nicolls1 nicolls1 changed the title refactor: start using @lifeomic/react-client Start using @lifeomic/react-client Feb 14, 2024
@coveralls
Copy link

coveralls commented Feb 16, 2024

Pull Request Test Coverage Report for Build 7974737404

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 10 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 85.077%

Totals Coverage Status
Change from base Build 7905759218: 0.002%
Covered Lines: 3685
Relevant Lines: 4208

💛 - Coveralls

@nicolls1 nicolls1 marked this pull request as ready for review February 16, 2024 21:29
@nicolls1
Copy link
Contributor Author

I believe I updated all for all of the current features in @lifeomic/react-client but let me know if I missed any

@@ -6,8 +6,8 @@ import axios, {
} from 'axios';
import { useAuth } from './useAuth';
import { APIClient } from '@lifeomic/one-query';
import { RestAPIEndpoints } from '../types/rest-types';
Copy link
Member

Choose a reason for hiding this comment

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

Can we now delete the types defined in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, +1 -- there are several big type defs defined in this project that should be deleted.

  • fhir api types
  • rest api types
  • auth api types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a bunch of test errors come up when deleting and moving over so I assumed we needed both but will remove.

@@ -104,6 +113,36 @@ export const useConsent = () => {
useConsentDirectives,
useUpdateProjectConsentDirective,
useShouldRenderConsentScreen,
} as {
Copy link
Member

Choose a reason for hiding this comment

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

Why did we need this cast?

Copy link
Contributor Author

@nicolls1 nicolls1 Feb 19, 2024

Choose a reason for hiding this comment

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

See slack or the failed earlier builds. Happy to fix some other way but that is what was decided there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not building the FL member app as an SDK is probably the biggest difference.

Comment on lines +36 to +45
Route extends keyof Endpoints & string,
Data = Endpoints[Route]['Response'],
>(
route: Route,
payload: RequestPayloadOf<Endpoints, Route>,
options?: RestrictedUseQueryOptions<
Endpoints[Route]['Response'],
unknown,
Data
>,
Copy link
Member

Choose a reason for hiding this comment

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

Why did we need this explicit type? I think we didn't have it when we used the library on the FL member app.

Copy link
Contributor Author

@nicolls1 nicolls1 Feb 19, 2024

Choose a reason for hiding this comment

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

See slack or the failed earlier builds. Happy to fix some other way but that is what was decided there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not building the FL member app as an SDK is probably the biggest difference.

@nicolls1
Copy link
Contributor Author

To be clear, tests are working but the build is what is requiring these explicit types because of this setting ts config setting "declaration": true,. I am not able to find another solution but open for suggestions.

@swain swain merged commit 2e8e8da into master Feb 21, 2024
3 checks passed
@swain swain deleted the FLME-639 branch February 21, 2024 15:28
Copy link

🎉 This PR is included in version 11.17.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Has been released to the package repository (NPM, etc) label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Has been released to the package repository (NPM, etc)
Projects
None yet
4 participants