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

Add option to generate ES Maps for protobuf map fields #1031

Open
JosephusPaye opened this issue Dec 3, 2024 · 3 comments
Open

Add option to generate ES Maps for protobuf map fields #1031

JosephusPaye opened this issue Dec 3, 2024 · 3 comments
Labels
Feature New feature or request

Comments

@JosephusPaye
Copy link

JosephusPaye commented Dec 3, 2024

Greetings!

Protobuf-ES currently generates plain JS objects for protobuf map fields, instead of Map instances. The manual mentions that this was decided for compatibility with "popular libraries":

ECMAScript Map objects have great support for key types, but many popular libraries don't support them correctly yet. For this reason, we use an object to represent map fields.

Maps have been supported by browsers and other JS runtimes for a while now (roughly since 2015), and from personal experience, seem to be widely supported across the ecosystem.

Are there any plans to revisit this decision? Alternatively, could an option be added that can be set to generate maps instead of objects? Something similar to what is available for 64-bit ints, where strings can be generated instead of bigints.

Thanks!

@JosephusPaye JosephusPaye changed the title Option to generate ES Maps for protobuf map fields Add option to generate ES Maps for protobuf map fields Dec 3, 2024
@timostamm timostamm added the Feature New feature or request label Dec 5, 2024
@timostamm
Copy link
Member

Hey Josephus, I can't give you an exhaustive list of popular libraries, but state management packages like Redux and frameworks like Next.js (SSR with getServerSideProps) don't support Maps yet.

An option to generate maps as Map might be a good solution.

@JosephusPaye
Copy link
Author

Yes, an option would be great to have.

@sbarfurth
Copy link

I took a quick-ish look at this. An option sounds easy on the surface, but this would require a new kind of option that propagates from protoplugin to protobuf. This is because descriptor/reflection based code in protobuf (e.g. create) will need to know what kind of code was generated by protoc-gen-es in order to behave correctly. One example is setting defaults when creating a new message from a schema (i.e. createZeroField).

I'm relatively sure no mechanism for communication from protoplugin to protobuf exists just yet, and in fact the dependency DAG is the other way around. One option I see would be to extend descFieldMapCommon with a hint about the generated map type for the reflection code and for protoplugin to set this during codegen based on an argument. I understand that this breaks the encapsulation of protoplugin, but I'm not sure I see another option if reflection code is meant to behave differently depending on codegen options. Using descFieldMapCommon is merely an example. Extending DescMessage would work almost the same way.

type descFieldMapCommon<T extends ScalarType = ScalarType> = T extends Exclude<ScalarType, ScalarType.FLOAT | ScalarType.DOUBLE | ScalarType.BYTES> ? {
  readonly fieldKind: "map";
  // other fields omitted...

  /** Whether the generated code expects an ES6 map or a plain object. Hint for reflection code. */
  es6Map: boolean;
} : never;

Wiring this will mean that protoplugin will need to pass something to the Registry it uses during code generation, since the registry initializes the Desc* types. This would need to be a public option on registry creation code. Client code could generate code with this option set and instantiate an arbitrary registry later without it, which would be a mismatch. This seems not great but I don't see a different way to do this. It's probably an edge case, but it seemed worth mentioning.

Even though this solution is not ideal (imho), the alternatives seem even more convoluted. It would also be possible to create options for relevant utilities (e.g. CreateOptions) to specify whether code was generated with ES6 maps. But this requires consumers to always set this option or risk runtime reflection errors.

Maybe someone else has a better idea or I may have overlooked something obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants