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

Optional any is typed as a required any. #230

Closed
rix0rrr opened this issue Sep 17, 2018 · 7 comments · Fixed by #237
Closed

Optional any is typed as a required any. #230

rix0rrr opened this issue Sep 17, 2018 · 7 comments · Fixed by #237
Assignees

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 17, 2018

Originally reported here:

aws/aws-cdk#714

The issue is that the declaration was:

interface Permission {
    sourceAccount?: any;
}

Which got translated into the following JSII:

        {
          "name": "sourceAccount",
          "type": {
            "primitive": "any"
          }
        },

I.e., the "optional" part of it got lost.

@RomainMuller RomainMuller self-assigned this Sep 18, 2018
@RomainMuller
Copy link
Contributor

I found the bug's cause (compiler would not consider the declaration.questionMark, and would only report as optional the union types including null and/or undefined). This actually makes me wonder if we should treat all any as optional (aka - if the type is any, then it is optional any)?

RomainMuller added a commit that referenced this issue Sep 18, 2018
Optional parameters and properties typed `any` would be represented as
required, despite the code unambiguously suggests the opposite. This is
due to the fact that `any` implicitly covers `null` and `undefined`.
This change fixes this by adding a specific provision for the question
mark token in the declarations of those, and adds compliance test
coverage for the same.

Fixes #230
@RomainMuller
Copy link
Contributor

Well the PR #237 includes code to make any optional everywhere it is found (because that is how TypeScript thinks of it).

@eladb
Copy link
Contributor

eladb commented Sep 18, 2018

But what happens if there’s an “any” argument in the middle of a signature? Optional arguments are only allowed at the end.

@RomainMuller
Copy link
Contributor

Well - optional is a type attribute, not a parameter attribute. In the general sense, it denotes nullability (and in fact, TypeScript lets you put optional arguments in any place you want).

RomainMuller added a commit that referenced this issue Sep 19, 2018
* fix(jsii): Optional `any` represented as required

Optional parameters and properties typed `any` would be represented as
required, despite the code unambiguously suggests the opposite. This is
due to the fact that `any` implicitly covers `null` and `undefined`.
This change fixes this by adding a specific provision for the question
mark token in the declarations of those, and adds compliance test
coverage for the same.

Fixes #230

* Mark all `any` types as `optional`.
eladb pushed a commit that referenced this issue Sep 20, 2018
Bug Fixes
======================
 * Sphinx generated incorrect type references for display ([#232](#232)) ([b664805](b664805))
* **jsii:** Defaulted parameters were not rendered as optional ([#234](#234)) ([578bf9c](578bf9c)), closes [#233](#233)
* **jsii:** Don't skip emit on TS errors when in "watch" mode ([#236](#236)) ([30d1491](30d1491)), closes [#235](#235)
* **jsii:** Optional `any` represented as required ([#237](#237)) ([91074f3](91074f3)), closes [#230](#230)

Features
======================
 * **sphinx:** allow readme file to define sphinx header and reorganize topic ([#229](#229)) ([405da9c](405da9c)), closes [#228](#228) [#185](#185)
* Document overriden/inherited members ([#238](#238)) ([7a6278a](7a6278a)), closes [#196](#196)
@eladb
Copy link
Contributor

eladb commented Sep 20, 2018

screen shot 2018-09-21 at 12 38 57 am

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Sep 21, 2018

Yes, there is (somewhat) of a difference between x?: l2 and x: l2 | undefined

@RomainMuller
Copy link
Contributor

That is in large parts because jsii models optional as a type attribute, and not as a parameter attribute...

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

Successfully merging a pull request may close this issue.

3 participants