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

Add persistence to API import forms #67

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Sep 17, 2024

Closes #62 (but not #63; auth-related fields are omitted here).

  • Introduces a new store named formSelections. This is populated when the application is initially launched, and saved in the browser's local storage session storage.
  • This stores the user's choices for the currently selected signal, as well as the options for each individual signal.

@@ -9,7 +9,7 @@ import {
fluViewRegions,
gftLocations,
ghtLocations,
nidssDenqueLocations,
nidssDengueLocations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NIDSS Dengue signal is referred to as "denque" in a few parts of the codebase. Because of this, it is currently broken on https://delphi.cmu.edu/epivis/. This PR also renames all mentions of "denque" to the correct "dengue".

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, but it is preferable to have this set of changes in its own PR... Could we break them out?

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! #68

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Can you merge #68 and then rebase this PR?

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

Instead of enumerating saved fields and setting their defaults for every endpoint component (plus the import dialog component) in src/components/dialogs/formSelections.ts, can we do it piece-by-piece inside each of the individual component definition files? There is already more obfuscation than there needs to be in this app (there is a lot of single-endpoint specific stuff in src/data/data.ts, /src/deriveLinkDefaults.ts and /src/api/EpiData.ts), and having structure and values in a location away from their only usage seems unnecessary. Do we even need all the EndpointFooSelections classes to be defined, or can the components treat the formSelections store as a generic object (where they can create and use their own sub-objects as they please)?

I came across this extension which you might be able to attach to the top-level form or to a <div> that wraps each component to get the persistence for ~free: https://github.com/fawaz-alesayi/svelte-use-persist

@@ -17,6 +18,11 @@ export const isShowingPoints = writable(defaults.showPoints);
export const initialViewport = writable(defaults.viewport);
export const navMode = writable(NavMode.autofit);

export const formSelections = writable(getFormSelections());
formSelections.subscribe((val) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this register a handler to unsubscribe when it goes out of scope or gets collected or whatever?

Copy link
Contributor Author

@rzats rzats Oct 9, 2024

Choose a reason for hiding this comment

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

That should be handled automatically. (Edit: I doublechecked and the subscribe function runs only once even if the "import API dialog" form is closed/opened/submitted a bunch of times)

Comment on lines 121 to 131
export function getFormSelections() {
try {
if (sessionStorage.getItem('form')) {
return JSON.parse(sessionStorage.getItem('form')!) as FormSelections;
}
return new FormSelections();
} catch {
sessionStorage.removeItem('form');
return new FormSelections();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

simpler:

Suggested change
export function getFormSelections() {
try {
if (sessionStorage.getItem('form')) {
return JSON.parse(sessionStorage.getItem('form')!) as FormSelections;
}
return new FormSelections();
} catch {
sessionStorage.removeItem('form');
return new FormSelections();
}
}
export function getFormSelections() {
try {
return JSON.parse(sessionStorage.getItem('form')!) as FormSelections;
} catch {
sessionStorage.removeItem('form');
}
return new FormSelections();
}

Also, this seems like it could/should go in src/store.ts since its only ever used 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.

I moved this to src/store.ts. However, the if statement should be kept - otherwise it returns JSON.parse(null) on initial load and causes some bad side effects.

@rzats
Copy link
Contributor Author

rzats commented Oct 9, 2024

Closing this PR - will implement svelte-use-persist in a separate PR instead; this uses sessionStorage internally, so the custom implementation is no longer needed.

@rzats rzats closed this Oct 9, 2024
@rzats rzats reopened this Oct 9, 2024
@rzats
Copy link
Contributor Author

rzats commented Oct 9, 2024

And reopening as the svelte-use-persist package does not appear to be functioning well out of the box:

  • the base import { persist } from 'svelte-use-persist'; import doesn't work because of some issues in the package's package.json
  • even if the code is brought into our environment as a typescript file, the persistent data is lost when the form is reloaded by tabbing in and out of the "import API" menu, even without reloading the page

So let's iterate a bit further on this PR. A couple of notes:

Instead of enumerating saved fields and setting their defaults for every endpoint component (plus the import dialog component) in src/components/dialogs/formSelections.ts, can we do it piece-by-piece inside each of the individual component definition files?

I could add these defaults to the components/dialogs/dataSources/*.svelte files, but the syntax for it is a bit awkward:

// needs to be a separate <script> tag
<script context="module" lang="ts">
  import { cdcLocations } from '../../../data/data';
  export class CdcSelections {
    locations = cdcLocations[0].value;
  }
</script>
<script lang="ts">
  // current code
</script>

This code also compiles and runs fine, but the current parser we use treats it as an error. (This can likely be fixed by modifying the config for it, though.)
Let me know if I should proceed with this approach anyway or if a different one could be used!

Do we even need all the EndpointFooSelections classes to be defined, or can the components treat the formSelections store as a generic object (where they can create and use their own sub-objects as they please)?

TypeScript/Svelte is fairly insistent on defining these sorts of types - I don't believe it's possible to cleanly use a key/value store like formSelections['cdcLocations'] instead of $formSelections.cdc.locations.

@rzats rzats requested a review from melange396 October 9, 2024 20:59
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.

Retain form choices in "Load" dialog
2 participants