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

feat: store last sent request in localstorage #350

Merged
merged 4 commits into from
Apr 21, 2021

Conversation

ValGeorgiev
Copy link
Contributor

@ValGeorgiev ValGeorgiev commented Apr 20, 2021

Store the last sent request in localStorage for both Extension and Electron apps.

Clean the localStorage when user use trash button

Issue: #328

Screen.Recording.2021-04-20.at.10.39.47.mov

Copy link
Collaborator

@andyrichardson andyrichardson left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Val! 🙌

A few quick thoughts on the implementation - let me know if you want to pair on any of this.

Handling state

Would it be possible to use a side effect for persisting to storage? Similarly, could we use that local storage value as the default value on mount?

This should allow us to avoid having storage-based logic throughout the app.

// Somewhere in useRequest
const state = useState({ query: window.localStorage.getItem('query') || '', /* ... */  });

useEffect(() => {
  window.localStorage.setItem('query', state.query);
}, [state.query]);

Storage mechanisms
This one is a little less straight forward and might need some investigation.

I'm guessing the expectation for persisting the query in localStorage is for these reasons:

  • User opens devtools and enters query
  • Query is persisted to storage
  • Next time user visits devtools, previous query is there

There's a potential problem with this which is that the query is being persisted to the extensions storage, but instead to an instance of the panel on a given tab. I'm not sure what this would result in but I would guess it would lead to unexpected results.

There is an API for browser extension storage that might be more work (likely including messaging to the background thread) but should reap expected results.

Note: If it does require messaging to the background thread - lets hold off until we work out whether we want to migrate to a service worker

For electron, it looks like local storage is fine 👍

If we end up using differing logic for storage, a util might make sense

// src/panel/util/storage.ts
export const write = (...args) => {
  if (process.env.BUILD_ENV === "extension") {
    // ...
  }
  localStorage.write(...args)
}

Edit: Spoke with @ValGeorgiev; localstorage will do the job - we're fine with having the value only persist for the lifetime of the tab

@ValGeorgiev
Copy link
Contributor Author

Thanks for doing this Val! 🙌

A few quick thoughts on the implementation - let me know if you want to pair on any of this.

Handling state

Would it be possible to use a side effect for persisting to storage? Similarly, could we use that local storage value as the default value on mount?

This should allow us to avoid having storage-based logic throughout the app.

// Somewhere in useRequest
const state = useState({ query: window.localStorage.getItem('query') || '', /* ... */  });

useEffect(() => {
  window.localStorage.setItem('query', state.query);
}, [state.query]);

Storage mechanisms
This one is a little less straight forward and might need some investigation.

I'm guessing the expectation for persisting the query in localStorage is for these reasons:

  • User opens devtools and enters query
  • Query is persisted to storage
  • Next time user visits devtools, previous query is there

There's a potential problem with this which is that the query is being persisted to the extensions storage, but instead to an instance of the panel on a given tab. I'm not sure what this would result in but I would guess it would lead to unexpected results.

There is an API for browser extension storage that might be more work (likely including messaging to the background thread) but should reap expected results.

Note: If it does require messaging to the background thread - lets hold off until we work out whether we want to migrate to a service worker

For electron, it looks like local storage is fine 👍

If we end up using differing logic for storage, a util might make sense

// src/panel/util/storage.ts
export const write = (...args) => {
  if (process.env.BUILD_ENV === "extension") {
    // ...
  }
  localStorage.write(...args)
}

Thanks @andyrichardson for the detailed comment. I updated the first part. I will have a look in the storage mechanisms now.

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.

None yet

2 participants