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 definition error #635

Closed
tpetry opened this Issue Jan 10, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@tpetry

tpetry commented Jan 10, 2017

Issue

TypeScript reports the following error when using strictNullChecks as already reported with #552 but closed without a solution:

node_modules/protobufjs/index.d.ts(1356,14): error TS2417: Class static side 'typeof Root' incorrectly extends base class static side 'typeof Namespace'.
  Types of property 'fromJSON' are incompatible.
    Type '(json: any, root?: Root | undefined) => Root' is not assignable to type '(name: string, json: { [k: string]: any; }) => Namespace'.
      Types of parameters 'root' and 'json' are incompatible.
        Type '{ [k: string]: any; }' is not assignable to type 'Root | undefined'.
          Type '{ [k: string]: any; }' is not assignable to type 'Root'.
            Property 'deferred' is missing in type '{ [k: string]: any; }'.

The definitions are:

export class Namespace extends ReflectionObject {
    /**
     * Constructs a namespace from JSON.
     * @param {string} name Namespace name
     * @param {Object.<string,*>} json JSON object
     * @returns {Namespace} Created namespace
     * @throws {TypeError} If arguments are invalid
     */
    static fromJSON(name: string, json: { [k: string]: any }): Namespace;
}

export class Root extends Namespace {
    /**
     * Loads a JSON definition into a root namespace.
     * @param {Object.<string,*>|*} json JSON definition
     * @param {Root} [root] Root namespace, defaults to create a new one if omitted
     * @returns {Root} Root namespace
     */
    static fromJSON(json: ({ [k: string]: any }|any), root?: Root): Root;
}

Versions

protobuf.js version: 6.4.5
typescript version: 2.1.4 (same with typescript@2.2.0-dev.20170110)

Reproduction

npm install protobufjs@6.4.5
npm install typescript@2.1.4
echo '{"compilerOptions":{"moduleResolution":"Node", "strictNullChecks":true, "target":"ES6"}}' > tsconfig.json
echo 'import * as protobuf from "protobufjs";' > index.ts
node_modules/.bin/tsc
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 10, 2017

Not being able to replace a static method like in this case seems just wrong to me. A Root instance simply doesn't have a name while all namespaces have one. I don't see any reason why this shouldn't be allowed in (anything that compiles to) JavaScript.

There are solutions for this, of course, and it will probably be necessary to implement one of these:

  1. Fix this in TypeScript
  2. Weaken our typings by replacing some stuff with any
  3. Weaken our documentation by adding a wrong base-overload to Namespace

We'll probably end up doing either 2. or 3. but then why exactly are we emitting TypeScript definitions again if we have to break them anyway... 😕

@tpetry

This comment has been minimized.

tpetry commented Jan 10, 2017

Conceptually you are right with omitting the name for a Root instance, but you are breaking fundamental OOP inheritance rules implemented in any language. Any inherited method can extend the method by adding anything or being more strict, but it can't loose any detail (a parameter in this case).

@tpetry

This comment has been minimized.

tpetry commented Jan 10, 2017

Are you ok with adding a new (non-exported) type class NamespaceMixin extends ReflectionObject with everything except the fromJSON method?

Then Namespace and Root could be changed to:

export class Namespace extends ReflectionObject implements NamespaceMixin {
    static fromJSON(name: string, json: { [k: string]: any }): Namespace;
}

export class Root ReflectionObject implements NamespaceMixin {
    static fromJSON(json: ({ [k: string]: any }|any), root?: Root): Root;
}
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 10, 2017

Just to follow up on your first reply: From a fundamental OOP perspective, you are absolutely correct. But what we have here is something that breaks only if a configuration options is present that is not specifically there to guarantee any fundamental laws of OOP - in a language that compiles to JavaScript, where there is absolutely no benefit of making this restriction. Don't get me wrong, I don't know much about the design principles behind TypeScript, I am just saying that having to deal with such an issue doesn't make a lot of sense to me in the first place.

On your second reply: Sure, I am open to cool hacks. That'd definitely be preferable over breaking something! Though, I am not sure how to integrate this into the automated JSDoc / .d.ts generation we have in place, yet.

dcodeIO added a commit that referenced this issue Jan 10, 2017

Moved TS-compatible Namespace features to a virtual NamespaceBase cla…
…ss, compiles with strictNullChecks by default now, see #635; Docs: Added SVG logo, see #629; Other: Minor codegen enhancements
@tpetry

This comment has been minimized.

tpetry commented Jan 10, 2017

But what we have here is something that breaks only if a configuration options is present that is not specifically there to guarantee any fundamental laws of OOP - in a language that compiles to JavaScript, where there is absolutely no benefit of making this restriction.

The design goal of TypeScript is to make programming more strict by introducing type safety (including an OOP system which JavaScript lacks). But i completely understand that not everybody likes these strict programming model.

Tested your commit fe4d97b and it solves the problem for every TypeScript developer. So now it's time to wait for the next tag to specify the new version as dependency.

@tpetry tpetry closed this Jan 10, 2017

dcodeIO added a commit that referenced this issue Jan 11, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 11, 2017

Now on npm as 6.4.6. Cheers!

@r-tock

This comment has been minimized.

Contributor

r-tock commented May 4, 2017

I am seeing this on 6.7.3 version now

ERROR in [at-loader] ../shared/node_modules/protobufjs/types/protobuf.js.d.ts:1285:11 
    TS2417: Class static side 'typeof Root' incorrectly extends base class static side 'typeof Namespace'.
  Types of property 'fromJSON' are incompatible.
    Type '(json: any, root?: Root | undefined) => Root' is not assignable to type '(name: string, json: { [k: string]: any; }) => Namespace'.
      Types of parameters 'root' and 'json' are incompatible.
        Type '{ [k: string]: any; }' is not assignable to type 'Root | undefined'.
          Type '{ [k: string]: any; }' is not assignable to type 'Root'.
            Property 'deferred' is missing in type '{ [k: string]: any; }'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment