Add BaseControllerV2 state metadata #362

Gudahtt · 2021-02-23T19:17:00Z

State metadata has been added to the new BaseController class as a required constructor parameter. The state metadata describes which pieces of state should be persisted, and how to get an 'anonymized' snapshot of the controller state.

This is part of the controller redesign (#337).

src/BaseControllerV2.ts

rekmarks · 2021-02-23T19:33:39Z

src/BaseControllerV2.ts

@@ -9,6 +9,15 @@ import type { Draft } from 'immer';
 */
 export type Listener<T> = (state: T) => void;

+export type Anonymizer<T> = (value: T) => T;


I'm not sure whether this is typed correctly. For example, let's say T is some interface, and the anonymizer deletes some keys on the object that implements T. In that case, I believe that the return value won't be T, and there will be much complaining from TypeScript.

I think this would be better:

Suggested change

export type Anonymizer<T> = (value: T) => T;

export type Anonymizer<T, U> = (value: T) => U;

Good point. I had only tested this in cases where the anonymized state is the same type.

Adding a second generic parameter doesn't really work though. What would we set it to?

Maybe something like T | Partial<T> would work, to at least let us omit properties. We'd have to preserve types for any properties that remain, but that might be OK.

Adding a second generic parameter doesn't really work though. What would we set it to?

It can either be set to Partial<T> by the caller (defaulting to that would be nice, but I don't know if that's possible), or I imagine something like this:

interface MyControllerState { ... } interface MyControllerAnonymousState { ... }

The simplest way to define the AnonymousState interface is probably by using the Omit utility type on MyControllerState, e.g. type MyControllerAnonymousState = Omit<MyControllerState, ...keys>.

If we add a second generic to Anonymizer, it would have to propagate to all of its consumers, so we'd ultimately end up with:

class MyController extends BaseController<MyControllerState, MyControllerAnonymousState> { ... }

Huh, that's an interesting idea! I'm not sure it's worth the effort at this point though, if we can find a default that works for all cases we can think of right now. I've been working on a recursive partial type that might be good enough for anything we'd want to do.

This is something we should keep in mind if we ever feel the need to work more with the anonymized state and want better typing, or if we want an anonymizer to return a totally different type at any point. It looks like TypeScript does support default generic parameters, so we might be able to do this in a way that doesn't burden every controller with having to declare this.

Here's what I have so far: 3f41181

Let me know what you think!

rekmarks · 2021-02-23T19:45:40Z

src/BaseControllerV2.ts

+  return typeof x === 'function';
+}
+
+export function getAnonymizedState<S extends Record<string, any>>(state: S, schema: Schema<S>) {


It'd be better for S to extend Record<string, unknown> as opposed to Record<string, any>. See here for details on unknown.

This is a change I believe that we should make everywhere in BaseControllerV2, and one we can do in a follow-up.

In addition, both getAnonymizedState and getPersistentState should probably take an additional generic type parameter, which should be returned instead of Partial<S>. If we always return Partial<S>, consumers will have to check for the existence of every key of S if they try to work with the return value of these functions.

Definitely in favour of using unknown over any!

I'm not sure about adding an additional generic type to those functions though 🤔. The main idea with the persisted and anonymous state is that we wouldn't be working with the return values. The persisted state gets persisted, and the anonymized state gets sent off as a state blob.

rekmarks · 2021-02-23T19:48:33Z

src/BaseControllerV2.ts

+  return typeof x === 'function';
+}
+
+export function getAnonymizedState<S extends Record<string, any>>(state: S, schema: Schema<S>) {


In addition, both getAnonymizedState and getPersistentState should probably take an additional generic type parameter, which should be returned instead of Partial<S>. If we always return Partial<S>, consumers will have to check for the existence of every key of S if they try to work with the return value of these functions.

Gudahtt · 2021-02-24T06:08:13Z

So, I've just had a realization. I think "schema" is the wrong word for this. This is more like state metadata - it doesn't describe the shape of the state. The state type does that.

Edit: This was addressed in 9817562

Gudahtt · 2021-02-25T05:11:23Z

I tried changing the state metadata type to allow arbitrary nesting, but I can't get past this last type error 🤔
The attempt is up as a draft PR: #364

@rekmarks

The BaseController state now uses `unknown` rather than `any` as the type for state properties. `unknown` is more type-safe than `any` in cases like this where we don't know what type to expect. See here for details [1]. This was suggested by @rekmarks during review of #362 [2]. [1]: microsoft/TypeScript#24439 [2]: #362 (comment)

rekmarks · 2021-02-25T19:18:36Z

@Gudahtt can you please summarize your thoughts on this?

Whether we want the anonymized/persistent state to be easy to work and harder to define, or easier to define but harder to work with.

Gudahtt · 2021-02-25T19:30:31Z

Sure! So mainly that was regarding your suggestions here and here. If we do something like you suggested and add another generic type parameter for the anonymized state (or the persisted state, or both), it would be easier for consumers of the anonymized and persisted state to work with it.

As you correctly pointed out earlier:

If we always return Partial<S>, consumers will have to check for the existence of every key of S if they try to work with the return value of these functions.

But my concern with that approach is that it'd make writing a controller more difficult, because there would be more types to declare upfront. It'd impact readability too - nobody wants to scroll for pages before getting to the actual controller code.

My assumption in going in the opposite direction - using Partial<S> (and later RecursivePartial<S>) - was that we wouldn't be doing anything with the anonymized state or the persisted state, so it's not particularly important how nice it is to work with. Using these types minimizes the work involved in declaring in the state metadata types, and they can be made more concise.

So those are the two paths as I see it - more concise metadata types with less descriptive derived state types, or more verbose metadata types for more descriptive derived state types.

We could pursue a third path by using less descriptive state types by default, but allowing the controller author to opt-in to using more descriptive types. I know TypeScript supports defaults for generic type parameters, but I don't have any idea whether this idea is feasible overall or how difficult it would be.

My suggestion for now would be to go with the first option (Partial<S> and RecursivePartial<S>) for now, and pursue this third path later if we find a reason to. Thoughts?

@rekmarks

* Use `unknown` rather than `any` for BaseController state The BaseController state now uses `unknown` rather than `any` as the type for state properties. `unknown` is more type-safe than `any` in cases like this where we don't know what type to expect. See here for details [1]. This was suggested by @rekmarks during review of #362 [2]. [1]: microsoft/TypeScript#24439 [2]: #362 (comment) * Use type alias for controller state rather than interface The mock controller state in the base controller tests now uses a type alias for the controller state rather than an interface. This was required to get around an incompatibility between `Record<string, unknown>` and interfaces[1]. The `@typescript-eslint/consistent-type-definitions` ESLint rule has been disabled, as this problem will be encountered fairly frequently. [1]: microsoft/TypeScript#15300 (comment)

A schema has been added to the new BaseController class as a required constructor parameter. The schema describes which pieces of state should be persisted, and how to get an 'anonymized' snapshot of the controller state. This is part of the controller redesign (#337).

Really this is state metadata, not a schema. The word "schema" never described this well.

The anonymizer function can now return a _recursive partial_ of the input type. So any properties can be omitted in the state property directly or in any nested object. Note that for primitive values and arrays, the return type must still match the state. Similarly, any objects still need to be returned as objects, though they're allowed to have a few less properties.

Note that the metadata type was split up to make it easier to use the `@property` JSDoc directive.

Tests have been added to ensure that the anonymizing function works as expected when the return type omits properties from the top-level property state or from a nested object.

The current state metadata takes an all-or-nothing approach to persisting each state property. There is no mechanism for omitting nested properties from the persisted state. As such, `Partial` is a better description of the return type than `RecursivePartial`.

rekmarks · 2021-02-25T20:41:27Z

@Gudahtt thanks!

We could pursue a third path by using less descriptive state types by default, but allowing the controller author to opt-in to using more descriptive types. I know TypeScript supports defaults for generic type parameters, but I don't have any idea whether this idea is feasible overall or how difficult it would be.

My suggestion for now would be to go with the first option (Partial<S> and RecursivePartial<S>) for now, and pursue this third path later if we find a reason to. Thoughts?

I think it would be straightforward to add one or more generic parameters with defaults, which would enable us to have our cake and eat it, too. Therefore, I think that's worth a shot, regardless of whatever else we do.

Depending on how much trouble RecursivePartial<S> gives us, it might just be worth to default to Record<string, unknown>. My reasoning here being, if we're going to work with a particular persistent or anonymous states a lot, we should just explicitly type it. That does add some upfront costs, but we only have to pay them once, and we should have most of the interface written already in any event.

If we're not working with a persistent or anonymous state to any significant extent, defaulting to Record<string, unknown> and typecasting as necessary should be OK. If we're not working with persistent or anonymous states at all, then Record<string, unknown> is of course fully sufficient.

I have two additional thoughts:

I recently ran into some trouble on a snaps branch writing un-serializable values to localStorage. We should consider how to ensure that the persistent state types prevent us from including such values, as they result in very nasty errors at runtime.
The ideal case would be to infer/generate persistent and anonymous state types from the Schema/Metadata value. That would obviate the need for explicit interfaces and Partial/RecursivePartial types, but is possibly (probably?) too costly to attempt at the moment, if it is indeed possible. Maybe @brad-decker has thoughts.

Gudahtt · 2021-02-25T21:01:21Z

I managed to get nested metadata / RecursivePartial working in this PR: #364
I needed to use a ts-ignore but I suspect that it is fully type-safe and that I'm running into a limitation of the TypeScript compiler that I don't fully understand. So if that's the case, maybe the ts-ignore is fine for now.

I recently ran into some trouble on a snaps branch writing un-serializable values to localStorage. We should consider how to ensure that the persistent state types prevent us from including such values, as they result in very nasty errors at runtime.

Already done! #366

Gudahtt · 2021-02-26T13:03:14Z

I'm going to move this back into draft for now, and pursue a simpler solution that only allows metadata for top-level keys, and uses Record<string, unknown>. I found that #364 couldn't handle one of our most common state patterns, so it needs more work.

Edit: The simpler state metadata has been implemented here: #371

Gudahtt · 2021-02-26T20:20:46Z

Closing this in favour of #371, which is a much simpler and more flexible implementation.

I might pursue the idea of a more declarative metadata format later on. I think it's possible for it to work nicely, but it's not easy.

@rekmarks

* Use `unknown` rather than `any` for BaseController state The BaseController state now uses `unknown` rather than `any` as the type for state properties. `unknown` is more type-safe than `any` in cases like this where we don't know what type to expect. See here for details [1]. This was suggested by @rekmarks during review of #362 [2]. [1]: microsoft/TypeScript#24439 [2]: #362 (comment) * Use type alias for controller state rather than interface The mock controller state in the base controller tests now uses a type alias for the controller state rather than an interface. This was required to get around an incompatibility between `Record<string, unknown>` and interfaces[1]. The `@typescript-eslint/consistent-type-definitions` ESLint rule has been disabled, as this problem will be encountered fairly frequently. [1]: microsoft/TypeScript#15300 (comment)

@rekmarks

* Use `unknown` rather than `any` for BaseController state The BaseController state now uses `unknown` rather than `any` as the type for state properties. `unknown` is more type-safe than `any` in cases like this where we don't know what type to expect. See here for details [1]. This was suggested by @rekmarks during review of #362 [2]. [1]: microsoft/TypeScript#24439 [2]: #362 (comment) * Use type alias for controller state rather than interface The mock controller state in the base controller tests now uses a type alias for the controller state rather than an interface. This was required to get around an incompatibility between `Record<string, unknown>` and interfaces[1]. The `@typescript-eslint/consistent-type-definitions` ESLint rule has been disabled, as this problem will be encountered fairly frequently. [1]: microsoft/TypeScript#15300 (comment)

Gudahtt requested a review from a team as a code owner February 23, 2021 19:17

Gudahtt mentioned this pull request Feb 23, 2021

Add BaseControllerV2 schema #359

Closed

rekmarks requested changes Feb 23, 2021

View reviewed changes

rekmarks reviewed Feb 23, 2021

View reviewed changes

rekmarks requested changes Feb 23, 2021

View reviewed changes

Gudahtt force-pushed the add-base-controller-schema branch 2 times, most recently from 6cc40d7 to 9a67b9e Compare February 24, 2021 13:39

Gudahtt changed the title ~~Add BaseControllerV2 schema~~ Add BaseControllerV2 state metadata Feb 24, 2021

Gudahtt force-pushed the add-base-controller-schema branch from 56b9d1b to 2458c4e Compare February 25, 2021 05:17

Gudahtt mentioned this pull request Feb 25, 2021

Use unknown rather than any for BaseController state #365

Merged

Gudahtt added 8 commits February 25, 2021 16:04

Rename scheme to metadata

64c910f

Really this is state metadata, not a schema. The word "schema" never described this well.

Add doc comments for all public functions and types

e2b22c2

Note that the metadata type was split up to make it easier to use the `@property` JSDoc directive.

Add tests for recursive partial anonymizing function

3e82037

Tests have been added to ensure that the anonymizing function works as expected when the return type omits properties from the top-level property state or from a nested object.

Fix mistake in RecursivePartial type

d113433

Fix typo and use T for generic for consistency

ef02ccc

Gudahtt force-pushed the add-base-controller-schema branch from 4b499b6 to ef02ccc Compare February 25, 2021 19:34

Gudahtt marked this pull request as draft February 26, 2021 13:03

Gudahtt closed this Feb 26, 2021

Gudahtt deleted the add-base-controller-schema branch February 27, 2021 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BaseControllerV2 state metadata #362

Add BaseControllerV2 state metadata #362

Gudahtt commented Feb 23, 2021 •

edited

Loading

rekmarks Feb 23, 2021

Gudahtt Feb 24, 2021

Gudahtt Feb 24, 2021

rekmarks Feb 24, 2021

Gudahtt Feb 24, 2021

Gudahtt Feb 24, 2021

rekmarks Feb 23, 2021

rekmarks Feb 23, 2021

Gudahtt Feb 24, 2021

rekmarks Feb 23, 2021

Gudahtt commented Feb 24, 2021 •

edited

Loading

Gudahtt commented Feb 25, 2021

rekmarks commented Feb 25, 2021

Gudahtt commented Feb 25, 2021

rekmarks commented Feb 25, 2021

Gudahtt commented Feb 25, 2021

Gudahtt commented Feb 26, 2021 •

edited

Loading

Gudahtt commented Feb 26, 2021

	export type Anonymizer<T> = (value: T) => T;
	export type Anonymizer<T, U> = (value: T) => U;

Add BaseControllerV2 state metadata #362

Add BaseControllerV2 state metadata #362

Conversation

Gudahtt commented Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt commented Feb 24, 2021 • edited Loading

Gudahtt commented Feb 25, 2021

rekmarks commented Feb 25, 2021

Gudahtt commented Feb 25, 2021

rekmarks commented Feb 25, 2021

Gudahtt commented Feb 25, 2021

Gudahtt commented Feb 26, 2021 • edited Loading

Gudahtt commented Feb 26, 2021

Gudahtt commented Feb 23, 2021 •

edited

Loading

Gudahtt commented Feb 24, 2021 •

edited

Loading

Gudahtt commented Feb 26, 2021 •

edited

Loading