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

6.7.0 appears to introduce breaking changes to TypeScript tooling #741

Closed
johnnyreilly opened this issue Apr 3, 2017 · 11 comments
Closed

Comments

@johnnyreilly
Copy link

protobuf.js version: 6.7.0

Hello!

We started experiencing failing builds this morning following the release of v6.7. With 6.7 in play we start to receive errors of this nature:

ERROR in ./src/stores/SomethingStore.ts
(66,65): error TS2345: Argument of type 'Something$Properties' is not assignable to parameter of type 'Something'.
  Property 'toObject' is missing in type 'Something$Properties'.

It looks as though the generated classes which previously contained payloads of a given type, eg Something, now contain payloads of a type like Something$Properties. This may be more correct if the payloads received only contain the properties (I don't know if this is the case). Either way, this change has broken our code as it depends upon the entities being Something rather than Something$Properties.

We're going to pin to 6.6.5 for now since that should resolve our immediate issue. But I wanted to raise it as it seems to be "breaking changes" if only from the TypeScript angle.

Also I wanted to clarify whether: the new tooling is more correct now or if this is a bug? I'm sorry that I can't share any code beyond my example code above; I'm happy to answer questions though.

Thanks for all your work!

@dcodeIO
Copy link
Member

dcodeIO commented Apr 3, 2017

Relevant issue: #717

Any instance of Something should satisfy Something$Properties without issues, though. $Properties is also only used as a parameter type and not returned, so additional information on where exactly the error originates would be helpful.

@johnnyreilly
Copy link
Author

johnnyreilly commented Apr 3, 2017

We have the concept of a ServerMessage which contains all the possible data structures that can be sent to the client. Here's an obfuscated version:

class ServerMessage {

    /**
     * Constructs a new ServerMessage.
     * @exports MyNamespace.ServerMessage
     * @constructor
     * @param {MyNamespace.ServerMessage$Properties=} [properties] Properties to set
     */
    constructor(properties?: MyNamespace.ServerMessage$Properties);

    /**
     * ServerMessage Somethings.
     * @type {Array.<MyNamespace.Something$Properties>|undefined}
     */
    public Somethings?: MyNamespace.Something$Properties[];

    // ...
}

You can hopefully see that the nested entities are returned as Something$Properties. Is that intentional?

This upsets the TypeScript compiler which is expecting a Something which has methods etc on it such as toObject that it may want to use.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 3, 2017

You can hopefully see that the nested entities are returned as Something$Properties. Is that intentional?

Ah, yes, that is intentional. When creating a message, submessages can be plain objects and hence are not necessarily an instance of the message class. The README now covers this in more detail (see also: Valid Message).

@johnnyreilly
Copy link
Author

johnnyreilly commented Apr 3, 2017

OK. So can we rely upon the $Properties suffix not changing in future (semver allowing)? If so then it would probably make sense to replace all our functions that receive the Something entity (in the case of nested entities) with Something$Properties instead. (If you see what I mean.)

dcodeIO added a commit that referenced this issue Apr 3, 2017
@dcodeIO
Copy link
Member

dcodeIO commented Apr 3, 2017

This now gives you an option --force-message to force documentation to reference actual message types instead of $Properties where applicable. It's still a bit stricter than before:

$Properties is still there for the constructor and .create, but it now also enforces the provided plain object to exclusively contain actual instances instead of $Properties, which is a requirement to be able to document field types as actual message types instead of $Properties (the constructor and .create just copy those over).

Hence, with --force-message, the new type checking is merely an improvement over 6.6.5, while it is a breaking change without.

dcodeIO added a commit that referenced this issue Apr 3, 2017
@johnnyreilly
Copy link
Author

Thanks @dcodeIO - I'll give it a try!

@johnnyreilly
Copy link
Author

Looks good - thanks for your help one again @dcodeIO! 👍

@johnnyreilly
Copy link
Author

Actually - I there's a problem; it looks like the Long definition is no longer referenced as I get this:

ERROR in E:\Source\App\proto\proto.d.ts
(157,25): error TS2304: Cannot find name 'Long'.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

Yeah, either install @types/long as a dependency of your project, or use the respective stub.

See: https://github.com/dcodeIO/protobuf.js#usage-with-typescript

@dcodeIO
Copy link
Member

dcodeIO commented Apr 11, 2017

6.7.3 includes typings for long and node again.

For reference, see these issues: #711 #753

So, this should be sorted in your favor now. Feel free to reopen if it is not!

@dcodeIO dcodeIO closed this as completed Apr 11, 2017
@johnnyreilly
Copy link
Author

Thanks!

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

No branches or pull requests

2 participants