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

Feature: Consider adding a more explicit SerializedMessage<T> type for the output of toJson #928

Closed
is-jonreeves opened this issue Jul 6, 2024 · 2 comments

Comments

@is-jonreeves
Copy link

Problem

The Type that is output from the toJson method obviously strips any useful information about the original object. This isn't a big deal and is likely expected, but wondered if it was really necessary?

Context

I have a react + redux-toolkit project that is currently using grpc-web via thunks and decided to migrate over to protobuf-es. I ran into an issue storing the protobuf-es objects, as they are instances (with complex field types) and require serialization before storing in Redux.

Thankfully you guys have a toJson method, but it strips all type-safety from the output. I guess this is somewhat expected, and I suspect the expectation is to use ObjectDto.fromJson() on the way out of the redux selectors to restore the instances, but it did create some confusion while working with the serialized object in the reducers (as they no longer have their types). This also means that the Type definition of the store slice has to be be made somewhat loose too.

One option is to first return a serialized output from the thunk (losing any type inference on the action), then deserialize in the reducer, perform any mutations and then reserialize again before storing as state. And then deserialize at the selectors.

While this is doable, losing the type inference of the action and having to deserialize and reserialize is a bit frustrating. This can have a broader impact if you are using dispatch(...).unwrap() and expect to get a typed response directly out of the actions.

An Alternative Approach

In my particular case, the only fields that pose a serializable issue are the Timestamp ones, and I noticed that toJson converts these to DateTime Strings. I decided to create a Type similar to your PlainMessage one to help out (and infers to the converted types of Date/Timestamp, BigInt and Uint8Array):

export type SerializedMessage<T extends Message<T>> = {
  [P in keyof T as T[P] extends Function ? never : P]: SerializedField<T[P]>;
};

type SerializedField<F> = F extends (Date | Timestamp) ? string
  : F extends Uint8Array ? number[]
  : F extends bigint ? number
  : F extends (boolean | string | number) ? F
  : F extends Array<infer U> ? Array<SerializedField<U>>
  : F extends ReadonlyArray<infer U> ? ReadonlyArray<SerializedField<U>>
  : F extends Message<infer U> ? SerializedMessage<U>
  : F extends OneofSelectedMessage<infer C, infer V> ? { case: C; value: SerializedField<V> }
  : F extends { case: string | undefined; value?: unknown } ? F
  : F extends { [key: string | number]: Message<infer U> } ? { [key: string | number]: SerializedField<U> }
  : F;

type OneofSelectedMessage<K extends string, M extends Message<M>> = { case: K; value: M };

In my case, I am using it in a preconfigured helper function like this:

type ToJson = {
  <T extends Message<T>>(message: Message<T>): SerializedMessage<T>
  <T extends Message<T>>(message: Message<T>[]): SerializedMessage<T>[]
}

export const toJson: ToJson = <T extends Message<T>>(message: any) => {
  if (Array.isArray(message)) return message.map(toJson);
  return message.toJson({ enumAsInteger: true, emitDefaultValues: true });
};

The Redux slice looks a little like this:

...
import { toJson } from '@/utils/grpc';

// State Type
interface SessionState {
  user: SerializedMessage<UserDto> | null
  ...
}

// Default State
export const initialState: SessionState = {
  user: null,
  ...
};

// Thunks
export const verifySession = createAsyncThunk('session/verify', async (_, thunkAPI) => {
  try {
    ...
    const response = await APIClients.users.getCurrentUser(request, { headers });
    return toJson(response)?.currentUser ?? null;
  } catch (err) {
    return thunkAPI.rejectWithValue(false);
  }
});

// Slice
export const sessionSlice = createSlice({
  name: 'session',
  initialState,
  reducers: {...},
  extraReducers: (builder) => {
    ...
    builder.addCase(verifySession.fulfilled, (state, action) => {
      state.user = action.payload.user;
    });
    ...
  },
};

// Base Selectors
const getUser = (state: RootState) => state.session.user;

// Memoized Selectors
export const selectUser = createSelector(getUser, user => UserDto.fromJson(user));

Suggestion

While I'm not 100% certain this is the best approach, it does seem to be working well for me, for now. It did make me wonder if it was worth adding this kind of Type inference to built into the outputs from the native toJson method? Or if there is a good reason not to? Or maybe if there is a better option to my Redux issues that I just missed?

@is-jonreeves
Copy link
Author

My bad, I see this might be a Duplicate of #508. And that a PR merged to potentially resolve this #866.

I guess I should download the v2 beta and give it a whirl

@timostamm
Copy link
Member

Hey Jon, thanks for the detailed issue!

The root cause here is that redux expects plain objects, while we use classes. The upcoming v2 should solve this problem - it uses plain objects for proto3 messages, which satisfies redux's constraints.

Similar limitations exist in React Server Components and other frameworks. We expect this new approach to work out of the box in most cases.

The new JSON types feature is another useful addition to the toolset. If you use proto2, messages use the prototype chain to track field presence, which makes them non-plain objects. Going with JSON is a reliable solution in this situation.

Closing this, since the solution will be available as a stable v2 soon. If you give the v2 beta a try, please let us know how it works for you 🙂

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

No branches or pull requests

2 participants