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

"Well-known types" ignore jstype overrides #1050

Closed
vwillyams opened this issue Jan 6, 2025 · 1 comment
Closed

"Well-known types" ignore jstype overrides #1050

vwillyams opened this issue Jan 6, 2025 · 1 comment

Comments

@vwillyams
Copy link

vwillyams commented Jan 6, 2025

As discussed in the Slack: https://bufbuild.slack.com/archives/CRZ680FUH/p1735588208612999

When using managed mode like so:

managed:
  enabled: true
  override:
    - field_option: jstype
      value: JS_STRING

The type of google.protobuf.Timestamp.seconds does not get overridden to string and instead remains a bigint. This is not surprising as these types are pre-compiled. This also applies to e.g. google.protobuf.Duration.

Possible fixes:

  • Drop pre-compilation (how much time does this even save?). Though this would be a breaking change by default - might want to make it an optional override.
  • Swap out the types at runtime (is this possible currently?). This would allow us to continue pre-compiling the timestamp files without changes.
  • Support references to local copies of well-known types, also allowing these to override whatever content is provided by the pre-compiled copies. Maybe the simplest solution but feels like hacky undocumented behavior particularly re: interactions with managed mode. See below issue 3 however as this already happens to some degree.

Note that there are three separate but closely related issues in this area which likely need to be resolved at the same time, relating to local copies of google.protobuf.Timestamp:

  1. By default, the seconds field is also generated as bigint when generating local copies of google.protobuf.Timestamp - ignoring managed mode overrides much the same as the parent issue.
  2. If those local copies of google.protobuf.Timestamp are manually modified with [jstype = JS_STRING] then it will generate the seconds field as string, inconsistent with the above behavior provided by managed mode.
  3. Files which reference the local copy of google.protobuf.Timestamp will import the local generated TS for this file. However, the Schema for this file references the pre-compiled copy. This results in inconsistency between Message and Schema type when combined with issue 2. Possibly caused by checking the file registry rather than resolving the referenced file.

Note also that the generated Timestamp code is referenced by the Timestamp util, which outputs a bigint for seconds - this would ideally need to be changed in order to maintain compatibility between typedef and functionality.

@timostamm
Copy link
Member

Hey Victor,

managed mode does not modify the well-known types. It's sensible that it doesn't, since it would be likely to cause issues in various Protobuf implementations. For the same reason, it's not a good idea to vendor well-known types.

Drop pre-compilation

ts-proto, protobuf-ts, and the dart implementation do not ship pre-compiled well-known types, but all other Protobuf implementations do. Protobuf-ES ships WKT as well - for consistency, and also to reduce bundle sizes: It doesn't make sense to define the equivalent to a standard library multiple times.

Swap out the types at runtime

This is not supported.

Support references to local copies of well-known types

If you specify include_wkt: true in buf.gen.yaml, you'll get a local copy, and imports to that local copy instead of from @bufbuild/protobuf/wkt. But managed mode does not modify the well-known types.

Besides the well-known types, it's also not possible to set the jstype option for map fields. A map<string, int64> field will always use bigint for map values.

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

3 participants