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

Move the "optional" attribute from TypeReference to Parameter #296

Closed
RomainMuller opened this issue Nov 7, 2018 · 4 comments · Fixed by #432
Closed

Move the "optional" attribute from TypeReference to Parameter #296

RomainMuller opened this issue Nov 7, 2018 · 4 comments · Fixed by #432
Assignees
Labels
effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1

Comments

@RomainMuller
Copy link
Contributor

The specification models the optional attribute of parameters on their TypeReference instead of directly on the parameter. This creates a confusion between nullable and optional.

The specification needs to be modified so optional parameters carry an optional attribute, while TypeReferences carry a nullable attribute.

Related: #230 #284 #295

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 28, 2018

So I wanted a function like such:

method(anchor: Construct | undefined, realParameter: string, optionalParameter?: string) {
}

Because that method order made most sense (and yet we didn't necessarily have the first argument always available), but this was rejected.

I've had to reorder arguments, which is not a BIG deal but it was suboptimal to my mind.

@eladb
Copy link
Contributor

eladb commented Dec 30, 2018

@rix0rrr I think is a different issue than where we model optionality of parameters. This is basically about nullibility of parameters, which is currently not supported by jsii (and should in my mind). Care to raise a separate issue?

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 31, 2018

Isn't adding type nullability something that we need to tackle as part of solving this ticket anyway?

@eladb
Copy link
Contributor

eladb commented Dec 31, 2018

it’s a related topic but not the same. This issue is basically just a modeling improvement (move the “optional” indication from the type reference to the parameter/property specification). Nullability is a different attribute of type references which is not the same as optionality.

If at all, nullability should be part of the type-reference while optionality should be part of the consuming specification.

@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
@RomainMuller RomainMuller self-assigned this Apr 4, 2019
@RomainMuller RomainMuller added the p1 label Apr 4, 2019
RomainMuller added a commit that referenced this issue Apr 4, 2019
Method `Parameters` now carry an `optional` flag that indicates whether
they are optional or required, and the `TypeReference#optional` field
was renamed to `TypeReference#nullable` to better reflect its semantics.

This also brings more flexibility in that it is now possible to model a
method with a nullable or defaulted argument that is followed by some
non-optional argument, and still obtain a reasonable type specification,
where previously this was an error.

Finally, in order to better reflect the type model of TypeScript and
Javascript, all `any` type references are now denoted `nullable`.

BREAKING CHANGE: JSII assemblies generated by older versions of the tool
will fail loading with this new version, and vice-versa. Re-compile your
projects in order to fix this.

Fixes #296
Fixes #414
@RomainMuller RomainMuller added the effort/large Large work item – several weeks of effort label Apr 8, 2019
RomainMuller added a commit that referenced this issue Apr 10, 2019
Method `Parameters`, `Properties` and method return types
now carry an `optional` flag that indicates whether they are
optional or required, and the `TypeReference#optional` field
was removed.

BREAKING CHANGE: JSII assemblies generated by older versions of the tool
will fail loading with this new version, and vice-versa. Re-compile your
projects in order to fix this.

Fixes #296
Fixes #414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants