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

typescript definitions meta-issue #718

Closed
tamird opened this Issue Mar 23, 2017 · 18 comments

Comments

Projects
None yet
2 participants
@tamird

tamird commented Mar 23, 2017

protobuf.js version: 6.6.5

First, thank you for including typescript support! I was delighted to discover that I could stop using https://github.com/sintef-9012/Proto2TypeScript.

However, there are a few problems with the generated and bundled TS definitions:

  • The included interface Long makes it impossible to chain Long methods on fields; e.g. d.timestampNanos.toNumber() becomes d.timestampNanos as Long).toNumber()
  • The generated type of certain numeric fields is number|Long, which also makes it impossible to chain Long methods; i.e. it becomes necessary to coalesce or cast to Long before being able to use the field value.
  • The bundled type definitions include hard-coded references to Buffer, which is only available in Node.js. In CockroachDB we are using protobuf.js in the browser, and would like to avoid including type definitions for Node.js types which will not be available at run time.
  • The generated definitions should also include interfaces from which protobuf objects can be constructed (distinct from the protobuf objects themselves). This is partially related to #719.
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

It seems that, no matter how this is done, that there'll always be some issues. For example, the minimal long interface has been introduced to handle the fact that long.js is optional. This solves the issue in most cases, but introduces new edge cases.

Similarly, previously all buffers were simply documented as being an Uint8Array, which is correct for newer versions of node, but requires explicit casts to use buffer functionality. So this has been changed to Buffer to make it work properly under node, and the typing referenced. Actually, protobuf.BufferReader and protobuf.BufferWriter are only used under node, though always included.

Related: #711

Long story short: I haven't found a way yet to make this work for all edge cases. Even generating multiple definition files, one for node and one for the browser, would introduce an inconvenient setup step to configure the correct file(s).

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

te minimal long interface has been introduced to handle the fact that long.js is optional

What's the issue? Perhaps a reasonable solution is simply not to make it optional? I'm sure there is context I'm missing.

Even generating multiple definition files, one for node and one for the browser, would introduce an inconvenient setup step to configure the correct file(s).

Seems to me that this is exactly what's needed, with a flag to pbts to import the desired one from generated code. You already do something similar with {light,minimal}.{js,d.ts}.

The other two issues I mentioned are with generated code - can they also be solved with flags to pbts?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

Perhaps a reasonable solution is simply not to make it optional?

Well, not everyone needs full 64 bit support (JS numbers are safe up to 53 bits). If long.js isn't available, protobuf.js falls back to using numbers - and the user can safe quite a bit of bundle size.

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

Ah, fair enough. In that case, this can also be solved with another variant a la {light,minimal}, correct?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

Hmm, well, yeah. I'd favor something generalized, but that might be the only working solution. We'd then have:

  • index
  • index-long
  • index-node
  • index-long-node
  • light
  • light-long
  • light-node
  • light-long-node
  • minimal
  • minimal-long
  • minimal-node
  • minimal-long-node

Hmm ....

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

That's a large matrix, but I don't know of another way. You'll also need to split the definitions (they currently are shared) to support real-long vs stub-long.

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

I've added another bullet to the list:

The generated definitions should also include interfaces from which protobuf objects can be constructed (distinct from the protobuf objects themselves). This is partially related to #719.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

Maybe also related: #717

If you have any exact requirements for .verify, .fromObject etc. feel free to join the discussion in 717. Currently, only constructors and .create are covered.

Edit: Sorry, needed multiple edits to get the referenced issue right.

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

Ah, looks like you just implemented that in #717. Awesome.

@dcodeIO dcodeIO added the enhancement label Mar 23, 2017

dcodeIO added a commit that referenced this issue Mar 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

This commit removes minimal type definitions from the library's default definitions and moves them to stubs instead:

Removing @types/node and/or @types/long would normally result in a tsc error, which can now be circumvented by referencing the stubs instead were either of these integrations is not wanted. For example, the following replaces both (browser only, no long.js):

/// <reference path="node_modules/protobufjs/stub-long.d.ts" />
/// <reference path="node_modules/protobufjs/stub-node.d.ts" />

With this in place we could now either create a matrix of entry points that'd only differ in their references or leave this up to the developer as a configuration step. Because of the amount of necessary entry points I'd favor the latter.

dcodeIO added a commit that referenced this issue Mar 23, 2017

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

Nice. I'll try this stuff out shortly. Just one issue remaining, AFAICT, and I've marked that in the issue description.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

The 2nd commit should cover this. It now emits Long instead of $protobuf.Long. Of course this leaves configuration completely up to the developer, but it should work.

@tamird

This comment has been minimized.

tamird commented Mar 23, 2017

dcodeIO added a commit that referenced this issue Mar 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

Well, this is still correct in some way. Even if you are not using long.js at all, you can still call a method with a proper LongStub (the thing now aliased as Long) as a parameter. As a return type, it doesn't make sense, though.

Possible workaround: Alias Long as a simple number, completely ignoring anything Long-like.

dcodeIO added a commit that referenced this issue Mar 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

This should do. Long now resolves as number, guaranteeing a consistent return type where long.js is not used. Likewise, where long.js is used, return types (not parameter types) can now be simplified to Long instead of number|Long. Looking into this now.

dcodeIO added a commit that referenced this issue Mar 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

This changes the return values. Note that messages properties can still be ´number|Long` because both is valid/supported as long as JS numbers are safe integers (<= 2^53).

dcodeIO added a commit that referenced this issue Mar 23, 2017

CLI: Added --strict-long option to pbjs to always emit 'Long' instead…
… of 'number|Long' (only relevant with long.js), see #718
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

This now basically does what you want, I believe:

Without long.js

  • Default is to output number|Long which coalesces to just number because stub-long.d.ts just aliases type Long = number
  • With --strict-long it outputs Long, which is shorter but still resolves to just number because of the above (more a cosmetic issue)

With long.js

  • Default is to output number|Long
  • With --strict-long it outputs Long only (this is the only functionally relevant option)

This does not affect return types of Reader-methods etc. (always uses Long as the return type, so it's actually a Long with long.js and a number without long.js through stub-long.d.ts)

Note that type Long = number just creates an alias that shows up as number in IntelliSense - unlike an interface, that would be displayed as the interface. This is also the reason why number|Long coalesces to just number with the alias in place.

@tamird

This comment has been minimized.

tamird commented Mar 24, 2017

Thanks so much! Going to close this in favour of the more specific #717 and #723.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment