Add web view state service and corresponding React hook

[lyonsil]

This change is 

[tjcouch-sil]

Wow!! Thanks for all the hard work on this! I really appreciate it. Looking forward to having this in! We're so close to being able to open a selected project in the resource viewer!!

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lyonsil)

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 4 at r1 (raw file):

// See src/shared/global-this.model.ts for normal hook documentation
// We don't add this to PAPI directly like other hooks because `this` has to be bound to a web view's iframe context

Since this doesn't go on the papi, I wonder if we should move it up a folder so people don't get confused and add it into the papi-exposed hooks on accident. No big deal either way.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 5 at r1 (raw file):

// See src/shared/global-this.model.ts for normal hook documentation
// We don't add this to PAPI directly like other hooks because `this` has to be bound to a web view's iframe context
export default function useWebViewState(

As it is currently coded, I believe the web view service and this hook don't support using the same key in two useWebViewState hooks in two different places. Probably not a problem right now, but it might be worth improving in the future. I made an issue #548 for it since it seems like we don't really need it right now.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 12 at r1 (raw file):

  stateKey: string,
): [webViewState: string, setWebViewState: Dispatch<SetStateAction<string>>] {
  const startingState: Record<string, string> = this.getWebViewState();

Because this is just a part of the function that is this hook, this will currently run every render. We could wrap this in a useMemo to make it only grab it once, but there is a more efficient way:

useState can take a function that returns the default state instead of just taking the default state itself, so I suggest this this.getWebViewState be moved into a function that is the argument for useState.

Do note, though, that setState only gets the initial value one time. You will also need a useEffect listening for changes to the stateKey that get the webview state again if the key changes. You can see an example of this in useSetting.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 13 at r1 (raw file):

): [webViewState: string, setWebViewState: Dispatch<SetStateAction<string>>] {
  const startingState: Record<string, string> = this.getWebViewState();
  const [state, setState] = useState(startingState[stateKey] ?? '');

This hook should be generic to accept the type for the state and should accept a defaultState parameter (the generic type can be inferred from the defaultState type, so people won't need to enter it).

useSetting has a good model for grabbing the initial value from somewhere and then setting to defaultState if it doesn't exist.

(Note that I discussed making web view state generic in comments on web-view-state.service.ts which would enable this to use generics as well)

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 15 at r1 (raw file):

  const [state, setState] = useState(startingState[stateKey] ?? '');
  // Whenever the webview state changes, save the updated value
  useEffect(() => {

Putting this in a useEffect causes it to happen another render later and causes it to happen when it shouldn't (it will set the state if you change the stateKey, which shouldn't happen). The recommended way to do this is to middle-man the setState function and do whatever background updates are needed in that middle-man function. You can see this in useSetting with setSettingInternal vs the setSetting that is returned.

---

src/renderer/services/web-view-state.service.ts line 13 at r1 (raw file):

  const entries = JSON.parse(serializedState) as [[string, Record<string, string>]];
  entries.forEach((item) => {

Generally when you're looping through this type of entries, it is best to use [key, value] or something along those lines in place of the parameter item - basically destructuring the entry into its constituent parts. Then it's easier to read what's going on.

like entries.forEach(([key, value]) => {

---

src/renderer/services/web-view-state.service.ts line 40 at r1 (raw file):

/** Set the web view state object associated with the given ID
 *  To avoid breaking references to existing state, call `getWebViewState` and modify the returned state object instead of setting a new state object

It took me a while to understand what this meant, and I could see people not implementing this correctly and getting problems. It makes me think these functions should probably also work with key/value pairs instead of web view state as a whole as it works in useWebViewState. I'd recommend going ahead and making that change now if that's ok since it would hopefully be a quick but very helpful change. It would also make #548 easier to do in the future (since it probably will require doing this work).

However, this could go in a follow-up issue if you don't want to do it that way. Let me know either way, and I can file an issue if you prefer (or maybe I'll just add it to #548)...?

---

src/renderer/services/web-view-state.service.ts line 42 at r1 (raw file):

 *  To avoid breaking references to existing state, call `getWebViewState` and modify the returned state object instead of setting a new state object
 */
export function setWebViewState(id: string, state: Record<string, string>): void {

Imposing that all the values are strings seems to cause issues with using web view state. It seems you encountered some difficulties in the hello world web view with clicks, which naturally should have been a number. Based on the code in this service, it seems the only restriction on the value types is that they should be JSON stringifiable, which is practically implied at this point since we impose that almost everywhere (but it probably would be worth noting in the JSDocs for webview-state-related functions, but our documentation on this requirement is pretty spotty already and wouldn't be a big deal if it isn't mentioned here).

I think it would be best to make these functions generic and have the state be the generic type. Then you can say it must be an object and default to Record<string, any>. Something like this:

function setWebViewState<TState extends object = Record<string, any>>(id: string, state: TState)

Then that would enable the useWebViewState hook to do its generics as I mentioned in comments on that file (Of course, the function signature I suggested assumes you don't change these functions to use key/value pairs).

---

src/shared/global-this.model.ts line 36 at r1 (raw file):

  var useWebViewState: (
    stateKey: string,
  ) => [webViewState: string, setWebViewState: Dispatch<SetStateAction<string>>];

We should add getWebViewState and setWebViewState here since they are exposed in the imports string in web-view.service.ts and may be used in WebView code, particularly if people want to use plain HTML webviews instead of React.

[lyonsil]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tjcouch-sil)

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 4 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Since this doesn't go on the papi, I wonder if we should move it up a folder so people don't get confused and add it into the papi-exposed hooks on accident. No big deal either way.

Done

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 12 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Because this is just a part of the function that is this hook, this will currently run every render. We could wrap this in a useMemo to make it only grab it once, but there is a more efficient way:

useState can take a function that returns the default state instead of just taking the default state itself, so I suggest this this.getWebViewState be moved into a function that is the argument for useState.

Do note, though, that setState only gets the initial value one time. You will also need a useEffect listening for changes to the stateKey that get the webview state again if the key changes. You can see an example of this in useSetting.

Done.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This hook should be generic to accept the type for the state and should accept a defaultState parameter (the generic type can be inferred from the defaultState type, so people won't need to enter it).

useSetting has a good model for grabbing the initial value from somewhere and then setting to defaultState if it doesn't exist.

(Note that I discussed making web view state generic in comments on web-view-state.service.ts which would enable this to use generics as well)

Done.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 15 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Putting this in a useEffect causes it to happen another render later and causes it to happen when it shouldn't (it will set the state if you change the stateKey, which shouldn't happen). The recommended way to do this is to middle-man the setState function and do whatever background updates are needed in that middle-man function. You can see this in useSetting with setSettingInternal vs the setSetting that is returned.

Everything seems to work properly with the code as it is. If stateKey does change (which I agree shouldn't happen generally) then this probably should save to the new key, shouldn't it?

I see what is done in useSetting, but I don't see how adding another middle-man actually helps anything here. It just seems like extra complexity. That might be different if there was a subscription involved (as it in useSetting), but there isn't at this point.

---

src/renderer/services/web-view-state.service.ts line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Generally when you're looping through this type of entries, it is best to use [key, value] or something along those lines in place of the parameter item - basically destructuring the entry into its constituent parts. Then it's easier to read what's going on.

like entries.forEach(([key, value]) => {

Done

---

src/renderer/services/web-view-state.service.ts line 40 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It took me a while to understand what this meant, and I could see people not implementing this correctly and getting problems. It makes me think these functions should probably also work with key/value pairs instead of web view state as a whole as it works in useWebViewState. I'd recommend going ahead and making that change now if that's ok since it would hopefully be a quick but very helpful change. It would also make #548 easier to do in the future (since it probably will require doing this work).

However, this could go in a follow-up issue if you don't want to do it that way. Let me know either way, and I can file an issue if you prefer (or maybe I'll just add it to #548)...?

Done

---

src/renderer/services/web-view-state.service.ts line 42 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Imposing that all the values are strings seems to cause issues with using web view state. It seems you encountered some difficulties in the hello world web view with clicks, which naturally should have been a number. Based on the code in this service, it seems the only restriction on the value types is that they should be JSON stringifiable, which is practically implied at this point since we impose that almost everywhere (but it probably would be worth noting in the JSDocs for webview-state-related functions, but our documentation on this requirement is pretty spotty already and wouldn't be a big deal if it isn't mentioned here).

I think it would be best to make these functions generic and have the state be the generic type. Then you can say it must be an object and default to Record<string, any>. Something like this:

function setWebViewState<TState extends object = Record<string, any>>(id: string, state: TState)

Then that would enable the useWebViewState hook to do its generics as I mentioned in comments on that file (Of course, the function signature I suggested assumes you don't change these functions to use key/value pairs).

Made this generic

---

src/shared/global-this.model.ts line 36 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We should add getWebViewState and setWebViewState here since they are exposed in the imports string in web-view.service.ts and may be used in WebView code, particularly if people want to use plain HTML webviews instead of React.

Done, but it doesn't make Intellisense work in the HTML web view code if that's what you had in mind.

[tjcouch-sil]

Some more react stuff to look at, but we can just make a follow-up issue if you prefer :) thanks for all the hard work on this! Great revisions!!

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/renderer/services/web-view-state.service.ts line 42 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Made this generic

Great!! Thank you :)

---

src/shared/global-this.model.ts line 36 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Done, but it doesn't make Intellisense work in the HTML web view code if that's what you had in mind.

It appears you put them in renderer/global-this.model.ts, but that unfortunately doesn't get carried into papi.d.ts; just this one in shared. However, since HTML web views don't currently have TypeScript support, maybe we can just mention to make this fix in #492 (I went ahead and made a note). Anyone who wants to use webview state in a ts file can probably get by with the hook for the time being.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 15 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Everything seems to work properly with the code as it is. If stateKey does change (which I agree shouldn't happen generally) then this probably should save to the new key, shouldn't it?

I see what is done in useSetting, but I don't see how adding another middle-man actually helps anything here. It just seems like extra complexity. That might be different if there was a subscription involved (as it in useSetting), but there isn't at this point.

Yes, saving new state when you set state works properly as-is. However, because of how the React rendering process works, useEffects run during the render once you set state. They don't happen immediately when you set the state. They are less efficient than simply executing code in functions, and I think (not absolutely confident) there is room for things to change such that the effect doesn't even run (like the component unmounting) so we would lose saving that state.

This section of the React docs about chaining computations pretty much explains what I'm talking about, but it's a lot more exaggerated than this situation.

I also expect we would get, not set, state when the stateKey changes. Turns out the hook is currently such that both happen in two different useEffects, which is confusing. However, it's not the end of the world to have a little bug, so we can make an issue on this if you'd prefer not to make further changes.

[irahopkinson]

Reviewed 2 of 9 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[lyonsil]

Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @irahopkinson and @tjcouch-sil)

---

src/shared/global-this.model.ts line 36 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It appears you put them in renderer/global-this.model.ts, but that unfortunately doesn't get carried into papi.d.ts; just this one in shared. However, since HTML web views don't currently have TypeScript support, maybe we can just mention to make this fix in #492 (I went ahead and made a note). Anyone who wants to use webview state in a ts file can probably get by with the hook for the time being.

You're right, that's my mistake. I mixed up the two global-this-model.ts files. Since it's mentioned in the ticket you referenced and HTML web views can't even use the types, I'll just let it be for now.

---

src/renderer/hooks/papi-hooks/use-webview-state.ts line 15 at r1 (raw file):
After pondering this a bit more, I'm going back to the previous version of this hook (except that I'm using your change for initialization). The second useEffect isn't really needed and just makes things more complex. This also aligns with other examples online. I know you made the following comment earlier, but I'm having a hard time seeing when/how this can happen and will cause a problem. I tested with multiple calls to the hook, and the unique values are loading and being set properly.

Do note, though, that setState only gets the initial value one time. You will also need a useEffect listening for changes to the stateKey that get the webview state again if the key changes. You can see an example of this in useSetting.

[tjcouch-sil]

Thanks for the hard work! So excited for this :)

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/shared/global-this.model.ts line 36 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

You're right, that's my mistake. I mixed up the two global-this-model.ts files. Since it's mentioned in the ticket you referenced and HTML web views can't even use the types, I'll just let it be for now.

Thanks for going ahead and moving it! Sorry to bother...