-
Notifications
You must be signed in to change notification settings - Fork 5
Manage localstorage cookies #720
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
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9f638f8
Make web implementation of PersistentConfigService
aswallace dc00711
Fix dispatch
aswallace e3cc57b
Dispatch tutorials from app
aswallace a414173
Add PersistentConfig to platformDependentServices
aswallace 1ff36bf
Merge branch 'main' of github.com:AllenInstitute/biofile-finder into …
aswallace 7a209b0
Merge branch 'main' of github.com:AllenInstitute/biofile-finder into …
aswallace 82c885b
Make state imports consistent
aswallace a776a98
Add version management to PersistentConfigService
aswallace fd24f53
Merge branch 'main' of github.com:AllenInstitute/biofile-finder into …
aswallace File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import * as React from "react"; | ||
| import { useDispatch, useSelector } from "react-redux"; | ||
|
|
||
| import { PersistedConfigKeys } from "../services"; | ||
| import { interaction, selection } from "../state"; | ||
|
|
||
| export default () => { | ||
| const dispatch = useDispatch(); | ||
| const { persistentConfigService } = useSelector( | ||
| interaction.selectors.getPlatformDependentServices | ||
| ); | ||
| React.useEffect(() => { | ||
| // Check localstorage cookies | ||
| const hasUsedApp = persistentConfigService.get( | ||
| PersistedConfigKeys.HasUsedApplicationBefore | ||
| ); | ||
| if (!hasUsedApp) { | ||
| // If first time using app, start running tutorials | ||
| dispatch(selection.actions.runAllTutorials()); | ||
|
|
||
| // Mark as true for next time | ||
| dispatch(interaction.actions.markAsUsedApplicationBefore()); | ||
| persistentConfigService.persist(PersistedConfigKeys.HasUsedApplicationBefore, true); | ||
| } | ||
| }, [dispatch, persistentConfigService]); | ||
| }; |
19 changes: 19 additions & 0 deletions
19
packages/core/services/PersistentConfigService/PersistentConfigServiceNoop.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import PersistentConfigService, { PersistedConfig } from "."; | ||
|
|
||
| export default class PersistentConfigServiceNoop implements PersistentConfigService { | ||
| public get() { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| public getAll(): PersistedConfig { | ||
| return {}; | ||
| } | ||
|
|
||
| public clear() { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| public persist() { | ||
| return Promise.resolve(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { lt, valid } from "semver"; | ||
|
|
||
| import { | ||
| PersistentConfigService, | ||
| PersistedConfig, | ||
| PersistedConfigKeys, | ||
| } from "../../../core/services"; | ||
|
|
||
| interface PersistentConfigServiceWebOptions { | ||
| clearExistingData?: boolean; | ||
| } | ||
|
|
||
| // Manually manage version | ||
| const CURRENT_VERSION = "1.0.0"; | ||
| const VERSION_KEY = "BFF_PERSISTED_CONFIG_VERSION"; | ||
|
|
||
| // Use browser localstorage to persist data between sessions | ||
| export default class PersistentConfigServiceWeb implements PersistentConfigService { | ||
| public constructor(options: PersistentConfigServiceWebOptions = {}) { | ||
| if (options.clearExistingData) { | ||
| localStorage.clear(); | ||
| } | ||
| // Check if localStorage has a stale version number | ||
| const storedVersion = localStorage.getItem(VERSION_KEY); | ||
| if (storedVersion !== CURRENT_VERSION) { | ||
| this.migrate(storedVersion); | ||
| // Update to new version | ||
| localStorage.setItem(VERSION_KEY, CURRENT_VERSION); | ||
| } | ||
| } | ||
|
|
||
| // Handle version migrations. | ||
| // If we deprecate keys in the future, we can use this method to | ||
| // remove/update values as needed, e.g., | ||
| // if (gt(oldVersion, "1.0.1")) { // old version > 1.0.1 | ||
| //. if (localStorage.getItem(PersistedConfigKeys.DeprecatedKey)) { | ||
| // localStorage.removeItem(PersistedConfigKeys.DeprecatedKey); | ||
| // } | ||
| // } | ||
| private migrate(oldVersion: string | null) { | ||
| // If invalid version, cannot validate stored keys | ||
| if (!oldVersion || lt(oldVersion, "0.0.0") || !valid(oldVersion)) { | ||
| this.clear(); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // localStorage only stores strings | ||
| public get(key: PersistedConfigKeys): string | undefined { | ||
| return localStorage.getItem(key) ?? undefined; // prefer undefined over null to match parent class | ||
| } | ||
|
|
||
| public getAll(): PersistedConfig { | ||
| return Object.values(PersistedConfigKeys).reduce( | ||
| (config: PersistedConfig, key) => ({ | ||
| ...config, | ||
| [key as string]: this.get(key), | ||
| }), | ||
| {} | ||
| ); | ||
| } | ||
|
|
||
| public clear(): void { | ||
| localStorage.clear(); | ||
| } | ||
|
|
||
| public persist(config: PersistedConfig): void; | ||
| public persist(key: PersistedConfigKeys, value?: string): void; | ||
| public persist(arg: PersistedConfigKeys | PersistedConfig, value?: any) { | ||
| if (typeof arg === "object") { | ||
| Object.entries(arg as PersistedConfig).forEach(([key, value]) => { | ||
| this.persist(key as PersistedConfigKeys, value); | ||
| }); | ||
| } else if (value === undefined || value === null) { | ||
| localStorage.removeItem(arg); | ||
| } else { | ||
| // setItem only accepts strings | ||
| localStorage.setItem(arg, value); | ||
| } | ||
| } | ||
| } | ||
87 changes: 87 additions & 0 deletions
87
packages/web/src/services/test/PersistentConfigServiceWeb.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import { expect } from "chai"; | ||
|
|
||
| import { PersistedConfigKeys } from "../../../../core/services"; | ||
| import PersistentConfigServiceWeb from "../PersistentConfigServiceWeb"; | ||
|
|
||
| describe("PersistentConfigServiceWeb", () => { | ||
| beforeEach(() => { | ||
| localStorage.clear(); | ||
| }); | ||
|
|
||
| describe("get", () => { | ||
| it("retrieves from localStorage", () => { | ||
| // Arrange | ||
| const expectedValue = "test value"; | ||
| const service = new PersistentConfigServiceWeb(); | ||
| localStorage.setItem(PersistedConfigKeys.HasUsedApplicationBefore, expectedValue); | ||
|
|
||
| // Act | ||
| const actualValue = service.get(PersistedConfigKeys.HasUsedApplicationBefore); | ||
|
|
||
| // Assert | ||
| expect(actualValue).to.equal(expectedValue); | ||
| }); | ||
|
|
||
| it("returns undefined when key does not exist", () => { | ||
| // Arrange | ||
| const service = new PersistentConfigServiceWeb({ clearExistingData: true }); | ||
|
|
||
| // Act | ||
| const actual = service.get(PersistedConfigKeys.ImageJExecutable); | ||
|
|
||
| // Assert | ||
| expect(actual).to.be.undefined; | ||
| }); | ||
| }); | ||
|
|
||
| describe("getAll", () => { | ||
| // Currently just a smoke test since we only store one key so far in web | ||
| it("retrieves all persisted keys", () => { | ||
| // Arrange | ||
| const service = new PersistentConfigServiceWeb(); | ||
| localStorage.setItem(PersistedConfigKeys.HasUsedApplicationBefore, "true"); | ||
|
|
||
| // Act | ||
| const persistedKeys = service.getAll(); | ||
| const keysWithValues = Object.values(persistedKeys).filter((val) => val != undefined); | ||
|
|
||
| // Assert | ||
| expect(Object.values(persistedKeys).length).to.equal( | ||
| Object.values(PersistedConfigKeys).length | ||
| ); | ||
| expect(keysWithValues.length).to.equal(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe("persist", () => { | ||
| it(`persists valid ${PersistedConfigKeys.HasUsedApplicationBefore}`, () => { | ||
| // Arrange | ||
| const service = new PersistentConfigServiceWeb({ clearExistingData: true }); | ||
| const expected = "true"; | ||
|
|
||
| // Act | ||
| service.persist(PersistedConfigKeys.HasUsedApplicationBefore, "true"); | ||
|
|
||
| // Assert | ||
| const actual = service.get(PersistedConfigKeys.HasUsedApplicationBefore); | ||
| expect(actual).to.equal(expected); | ||
| }); | ||
|
|
||
| it("clears keys when value is undefined", () => { | ||
| // Arrange | ||
| const service = new PersistentConfigServiceWeb(); | ||
|
|
||
| // consistency check | ||
| service.persist(PersistedConfigKeys.HasUsedApplicationBefore, "true"); | ||
| const intermediateValue = service.get(PersistedConfigKeys.HasUsedApplicationBefore); | ||
| expect(intermediateValue).to.equal("true"); | ||
|
|
||
| // Act | ||
| service.persist(PersistedConfigKeys.HasUsedApplicationBefore, undefined); | ||
| const actual = service.get(PersistedConfigKeys.HasUsedApplicationBefore); | ||
|
|
||
| // Assert | ||
| expect(actual).to.be.undefined; | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| // Make sure specific type definition APIs are available on compile. | ||
| // Specifically added for WebWorker API, but seems that listing that one | ||
| // makes adding ESNext also necessary | ||
| "lib": ["WebWorker", "ESNext"], | ||
| "lib": ["WebWorker", "ESNext", "DOM"], | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just because of some typescript errors showing up after a recent IDE update; makes this library explicitly available |
||
| "module": "ESNext", | ||
| "moduleResolution": "Node", | ||
| "preserveConstEnums": true, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other config storage keeps track of versions with schemas right? What do you think about storing the config version whenever this initializes so that we can version the config version (..) style going forward in case we end up changing the config variables a lot in the future? We could then, if the major version has changed, delete(?) the cookies saved so far and start anew. Worth checking if there is some existing convention for how apps do this too if you haven't yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me! The other version was specific to "electron-store", and I wasn't sure if there was an equivalent I could translate to local storage. I think it's worth doing though, will take a look.
On a similar topic, I didn't include the other cookies that we currently use in desktop, but may be worth discussing if we want any of those to be included in web (e.g., columns?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added version in a776a98