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

fix: typeof function #1182

Merged
merged 2 commits into from Jul 31, 2022
Merged

fix: typeof function #1182

merged 2 commits into from Jul 31, 2022

Conversation

loopingz
Copy link
Contributor

Fix issue #1181

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had requests for supporting functions before. Please search the issues and prs.

in general, I'm not sure I want to support functions since json schema doesn't support them. Maybe the cleaner way is to just ignore functions entirely?

test/valid-data/type-typeof-function/main.ts Outdated Show resolved Hide resolved
src/TypeFormatter/FunctionTypeFormatter.ts Outdated Show resolved Hide resolved
@loopingz
Copy link
Contributor Author

You are right I think they should just be ignored, but failing is not a good option

@loopingz
Copy link
Contributor Author

I did update to return an UnknownType with a $comment with the definition of the function.
I think it allows to explain why this property exists while waiting for one day maybe have a normalized way of expressing it with json-schema
If you agree with this approach, I can add it for a FunctionDeclarationNodeParser that would be more generic behavior

@domoritz
Copy link
Member

Remind me, is $comment standard? Should we add not: {} to indicate that the property should not be set?

@loopingz
Copy link
Contributor Author

The $comment seems a standard to JSONSchema7: http://json-schema.org/draft-07/schema properties.$comment

...
    "properties": {
        "$id": {
            "type": "string",
            "format": "uri-reference"
        },
        "$schema": {
            "type": "string",
            "format": "uri"
        },
        "$ref": {
            "type": "string",
            "format": "uri-reference"
        },
        "$comment": {
            "type": "string"
        },
....

That was the first behavior to add a not: {}, I moved to unknown with $comment, I m ok to go back to not, we could also add a not with the reasoning within the $comment

@domoritz
Copy link
Member

I think I would prefer no comment in the schema and instead a warning. JSON schema does not support functions and therefore we should not attempt to make them work.

@loopingz loopingz changed the title fix: typeof function (Close #1181) fix: typeof function Mar 28, 2022
@loopingz
Copy link
Contributor Author

I think adding a not: {} is already opiniated, maybe we could add an option to set how to manage function type:

  • IGNORE: remove completely the property
  • COMMENT: free declaration with a comment on the method signature
  • FORBID: add the not {}

With the available override capabilities, we can either just document how to code each behavior

@domoritz
Copy link
Member

I like the idea of documenting how to implement the behaviors. How about we implement ignore as the default?

@loopingz
Copy link
Contributor Author

Sounds good to me

@broofa
Copy link

broofa commented Jun 8, 2022

The 'Unknown type "function"' error is going to be pretty common. This is just one of those differences between TS and JSON that is inherent to this problem space. But the current behavior is pretty atrocious. Schema generation fails completely, and you get a really terse error message that has zero information about where the error occurred (The type name and property that triggered the problem is not supplied), and zero information about how to resolve the problem.

Developers are forced to sift through the issues for a solution. This takes you down the rabbit hole of having to write a custom generator, which is also not particularly well documented. (This is where I'm at right now, trying to figure out what exactly my generator code needs to look like and having some issues with that, unfortunately.)

I would much rather there were a simple way of indicating how unknown types should be handled as suggested in this issue. E.g. A config option along the lines of...

  "unknownTypes": {
    "function": "ignore",
    // ...
  }

@domoritz domoritz merged commit aa63c0a into vega:next Jul 31, 2022
@github-actions
Copy link

🚀 PR was released in v1.1.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Sep 21, 2022
@kingyue737
Copy link

kingyue737 commented Oct 23, 2022

I still faced the following error in v1.1.2:

Error: Unknown type "function"

Do I need to add any config to ignore function type?


EDITED:
This regression has been confirmed #1456

@ChromeQ
Copy link

ChromeQ commented Dec 26, 2022

Confirming this issue is still present in v1.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants