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.

Sign up for GitHub

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

Jump to bottom

PHC-4151 Update useActiveProject to read/write to async storage #68

Merged
merged 2 commits into from
Mar 9, 2023
Merged

PHC-4151 Update useActiveProject to read/write to async storage #68

merged 2 commits into from
Mar 9, 2023

Conversation

bruce-glazier
Copy link
Contributor

@bruce-glazier bruce-glazier commented Mar 3, 2023 •
edited
Loading

Changes

Screenshots

@bruce-glazier bruce-glazier changed the title Phc 4151 use active project async storage PHC-4151 Use active project async storage Mar 3, 2023
@bruce-glazier bruce-glazier changed the title PHC-4151 Use active project async storage PHC-4151 Store selected project id in async storage Mar 3, 2023
@bruce-glazier bruce-glazier changed the title PHC-4151 Store selected project id in async storage PHC-4151 Update useActiveProject to read/write to async storage Mar 3, 2023
@coveralls
Copy link

coveralls commented Mar 3, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4368773861

  • 39 of 40 (97.5%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 96.131%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/hooks/useAsyncStorage.ts 6 7 85.71%
Totals Coverage Status
Change from base Build 4367080534: -0.005%
Covered Lines: 522
Relevant Lines: 544
💛 - Coveralls

@bruce-glazier bruce-glazier force-pushed the PHC-4151-useActiveProject-async-storage branch from 6845898 to 32d8a2c Compare March 3, 2023 21:36
@bruce-glazier bruce-glazier force-pushed the PHC-4151-useActiveProject-async-storage branch from 32d8a2c to 60b829b Compare March 3, 2023 21:45
sternetj
sternetj previously approved these changes Mar 6, 2023
View reviewed changes
markdlv
markdlv reviewed Mar 6, 2023
View reviewed changes
src/hooks/useActiveProject.tsx Outdated
@@ -50,35 +59,54 @@ export const ActiveProjectContextProvider = ({
return;
}

// TODO: Save for previously-selected project
storeProjectId(selectedProject.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to await this, I'd think?

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 may want to catch any errors here but I'm not sure why we would need to wait for this to complete before proceeding. At this point, we've already set the activeAccountId in state so we really only need to store this for the next session. This goes to the below explanation about why I wrapped the get async storage with react-query, mostly to keep the effect cleaner without having to convert it to be async.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in this case, the callback is already async. And agreed- if you still feel we shouldn't wait/verify it's stored before moving on, we need to (whether in this PR or down the road) log the error or something so it doesn't just go unnoticed.

markdlv
markdlv previously approved these changes Mar 6, 2023
View reviewed changes
@bruce-glazier bruce-glazier force-pushed the PHC-4151-useActiveProject-async-storage branch from c4217cf to 40ca665 Compare March 7, 2023 19:12
namick
namick previously approved these changes Mar 8, 2023
View reviewed changes
src/hooks/useActiveProject.tsx Outdated
Comment on lines 40 to 54
if (!selectedProject || !selectedSubject) {
console.warn('Ignoring attempt to set invalid projectId', projectId);

// Attempt to fallback to first available project
const defaultProject = projects?.[0];
const defaultSubject = subjects?.find(
(s) => s.projectId === defaultProject?.id,
);
if (!defaultProject || !defaultSubject) {
console.warn('Failed attempt to select default projectId', projectId);
return { selectedProject: undefined, selectedSubject: undefined };
}
return { selectedProject: defaultProject, selectedSubject: defaultSubject };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

General question about how we want to use console.warn in this project. Are these warnings for us, or for the end consumer/developer who might be building with this SDK in their app? Obviously these are only shown in development mode. Also, isn't it a normal, supported case that a user may not have any projects, and thus a console warning might be noise?

The PR looks good though, not wanting to hold it up with my questions. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up a bit to not warn so often, should only warn if we explicitly set an invalid project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should add something like [LifeOmicSDK] prefixes to logs and errors so that user's can quickly identify where the error is coming from?

sternetj
sternetj reviewed Mar 8, 2023
View reviewed changes
src/hooks/useActiveProject.tsx Outdated
@@ -36,40 +65,63 @@ export const ActiveProjectContextProvider = ({
const [hookReturnValue, setHookReturnValue] = useState<ActiveProjectProps>(
{},
);
const { useGetItem, setItem } = useAsyncStorage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this hook return a hook? why cant we do something like

const [storedProjectIdResult, setStoredProjectId] = useAsyncStorage(selectedProjectIdKey);

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 can and we should, I like that even better.

src/hooks/useActiveProject.test.tsx Outdated
useAsyncStorageSpy = jest.spyOn(useAsyncStorage, 'useAsyncStorage');
});

test('initial render writes selected account to async storage', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test title correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps, nope, copy/paste error.

@bruce-glazier bruce-glazier force-pushed the PHC-4151-useActiveProject-async-storage branch from f24c645 to 951fea3 Compare March 8, 2023 21:39
bruce-glazier
bruce-glazier commented Mar 8, 2023
View reviewed changes
src/hooks/useAsyncStorage.ts
AsyncStorage.setItem(key, value);
const setItem = (value: string) =>
AsyncStorage.setItem(key, value).catch((error) =>
console.error(`[LifeOmicSDK]:${error}`),
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 think in a future PR I will purpose a wrapper or something around console to include that prefix.

sternetj
sternetj approved these changes Mar 9, 2023
View reviewed changes
src/hooks/useActiveProject.test.tsx
activeSubjectId: mockMe[1].subjectId,
});

useAsyncStorageSpy = jest.spyOn(useAsyncStorage, 'useAsyncStorage');
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to move this to beforeEach incase this test fails.

@bruce-glazier bruce-glazier merged commit 8933179 into master Mar 9, 2023
@bruce-glazier bruce-glazier deleted the PHC-4151-useActiveProject-async-storage branch March 9, 2023 15:42
@github-actions
Copy link

github-actions bot commented Mar 9, 2023

🎉 This PR is included in version 1.1.15 🎉

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 Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sternetj sternetj sternetj approved these changes

@namick namick Awaiting requested review from namick

@markdlv markdlv Awaiting requested review from markdlv

Assignees
No one assigned
Labels
released Has been released to the package repository (NPM, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bruce-glazier @coveralls @namick @markdlv @sternetj