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

Non-optional fields causing TypeScript errors #749

Closed
paralin opened this issue Apr 5, 2017 · 13 comments
Closed

Non-optional fields causing TypeScript errors #749

paralin opened this issue Apr 5, 2017 · 13 comments

Comments

@paralin
Copy link

paralin commented Apr 5, 2017

protobuf.js version: 6.7.1

I'm passing the output of pbjs json conversion to loadJson, this works fine, but there's a new typing error in the latest update:

Argument of type '{ "nested": { "google": { "nested": { "api": { "options": { "java_multiple_files": boolean; "java...' is not assignable to parameter of type 'NamespaceDescriptor'.
  Types of property 'nested' are incompatible.
    Type '{ "google": { "nested": { "api": { "options": { "java_multiple_files": boolean; "java_outer_class...' is not assignable to type '{ [k: string]: EnumDescriptor | ExtensionFieldDescriptor | ExtensionMapFieldDescriptor | TypeDesc...'.
      Property '"google"' is incompatible with index signature.
        Type '{ "nested": { "api": { "options": { "java_multiple_files": boolean; "java_outer_classname": strin...' is not assignable to type 'EnumDescriptor | ExtensionFieldDescriptor | ExtensionMapFieldDescriptor | TypeDescriptor | Servic...'.
          Type '{ "nested": { "api": { "options": { "java_multiple_files": boolean; "java_outer_classname": strin...' is not assignable to type 'ServiceDescriptor'.
            Property 'methods' is missing in type '{ "nested": { "api": { "options": { "java_multiple_files": boolean; "java_outer_classname": strin...'

The issue is that the "methods" field is not optional. Could you please set that field to optional in the typings?

Thanks!

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

methods is actually required, it marks an object to be a service. Without it, fromJSON doesn't know what kind of object it is handling.

There is a table of required properties of the different JSON structures here.

If you generated the respective JSON descriptor somehow, there might be something wrong in generation instead.

Alternatively, the issue could be more general in that the object does not match any valid type. For instance, if google is just a namespace, it should resemble an empty message (an object with a fields = {} property) within a JSON descriptor.

@paralin
Copy link
Author

paralin commented Apr 5, 2017

Right, but it's not required on every single thing, right? You can treat it not being there as null. If I just ignore the typing error protobufjs works correctly anyway right now.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

Yeah, without a marker, addJSON falls back to treat the object as a namespace. Inner namespaces aren't really intended, though, but could be added somewhere around here. Issue here is that nested is optional on NamespaceDescriptor because it's abstract, effectively making the type accept empty objects.

@paralin
Copy link
Author

paralin commented Apr 5, 2017

I nest namespaces in my code, quite a bit actually. It seems to work just fine! Is this just a typings issue, or does something need to be changed in ProtobufJS to support that pattern? Assuming the fallback is actually working as intended and I'm not abusing some kind of bug.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

More of a typings issue because of empty objects.

Issue here is that nested is optional on NamespaceDescriptor because it's abstract, effectively making the type accept empty objects.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

Currently trying to fix this, but as soon as I add a sixth type to

@type {EnumDescriptor|TypeDescriptor|ServiceDescriptor|ExtensionFieldDescriptor|ExtensionMapFieldDescriptor}

my VSCode hangs with 100% cpu. That's new. Lots of recursion if I had to guess.

@paralin
Copy link
Author

paralin commented Apr 5, 2017

My tslint did the same thing for the longest time, that's probably your issue. I think it has some unchecked infinite recursion if you use recursive structures in your code.

The issue is with only one of the checkers - the "unused variable" checker. But you're not even using tslint, I think. Your vscode might be using it internally. You could try turning off that checker in a little tslint.json file and see if it fixes it.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

Actually had a tslint plugin installed but the issue remains after uninstalling it. That's amazing...

@paralin
Copy link
Author

paralin commented Apr 5, 2017

eslint? Wait, no, you had to be using tslint, if the issue started happening after you changed the typings.

I would try dropping a tslint.json in the root of your repo, here's mine:

[removed]

There are some Angular2 rules in there that you'll get errors about but you can just delete those lines. The most important part is to turn off the unused variable analysis.

@paralin
Copy link
Author

paralin commented Apr 5, 2017

Here's the non-angular2 one with the unused variable checker disabled:

"no-unused-expression": false,

{
  "rules": {
    "class-name": true,
    "comment-format": [
      true,
      "check-space"
    ],
    "curly": true,
    "eofline": true,
    "forin": true,
    "indent": [
      true,
      "spaces"
    ],
    "label-position": true,
    "max-line-length": [
      true,
      140
    ],
    "member-access": false,
    "member-ordering": [
      true,
      "static-before-instance",
      "variables-before-functions"
    ],
    "no-arg": true,
    "no-bitwise": true,
    "no-console": [
      true,
      "debug",
      "info",
      "time",
      "timeEnd",
      "trace"
    ],
    "no-construct": true,
    "no-debugger": true,
    "no-duplicate-variable": true,
    "no-empty": false,
    "no-eval": true,
    "no-inferrable-types": true,
    "no-shadowed-variable": true,
    "no-string-literal": false,
    "no-switch-case-fall-through": true,
    "no-trailing-whitespace": true,
    "no-unused-expression": false,
    "no-use-before-declare": false,
    "no-var-keyword": true,
    "object-literal-sort-keys": false,
    "one-line": [
      true,
      "check-open-brace",
      "check-catch",
      "check-else",
      "check-whitespace"
    ],
    "quotemark": [
      true,
      "single"
    ],
    "radix": true,
    "semicolon": [
      "always"
    ],
    "triple-equals": [
      true,
      "allow-null-check"
    ],
    "typedef-whitespace": [
      true,
      {
        "call-signature": "nospace",
        "index-signature": "nospace",
        "parameter": "nospace",
        "property-declaration": "nospace",
        "variable-declaration": "nospace"
      }
    ],
    "variable-name": false,
    "whitespace": [
      true,
      "check-branch",
      "check-decl",
      "check-operator",
      "check-separator",
      "check-type"
    ]
  }
}

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

@dcodeIO
Copy link
Member

dcodeIO commented Apr 5, 2017

Well, I introduced a new otherwise useless type AnyExtensionFieldDescriptor covering two types making it a total of 5 again in AnyNestedDescriptor. Problem solved.

vscode issue: microsoft/vscode#23958

@dcodeIO
Copy link
Member

dcodeIO commented Apr 11, 2017

Should be working with 6.7.3. Feel free to reopen if it's not!

@dcodeIO dcodeIO closed this as completed Apr 11, 2017
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