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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

go: dealing with required props and nils #3438

Closed
1 of 7 tasks
polothy opened this issue Mar 22, 2022 · 2 comments
Closed
1 of 7 tasks

go: dealing with required props and nils #3438

polothy opened this issue Mar 22, 2022 · 2 comments
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@polothy
Copy link
Contributor

polothy commented Mar 22, 2022

馃殌 Feature Request

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)
  • Go

General Information

  • JSII Version:
  • Platform:
  • I may be able to implement this feature request
  • This feature might incur a breaking change

Description

Just some general feedback on what I'm seeing with the conversion of Props like interfaces to Go. This was maybe already discussed but I couldn't find anything (sorry).

First item, the jsii.{Type} functions, EG jsii.String(). I use these all the time with AWS SDK for Go and they're a real bummer to use. If you forget to call it, then you have to wrap your statement. Then if you don't know the type (was it int? int32? int64?) then you need to do some jumping around to figure it all out. I haven't found a good editor experience to make this "nice".

The other problem is it seems like folks are having a hard time figuring out required vs optional properties, see:

Proposed Solution

For the first problem, I 100% understand why this is there and the nils are necessary. With Go 1.18, you can now provide this code:

package jsii

// Of will return the reference of a variable
func Of[T any](v T) *T {
	return &v
}

// To will dereference a variable - if nil, returns zero value
func To[T any](v *T) T {
	// This is how you get default value of T
	var t T
	if v == nil {
		return t
	}
	return *v
}

I've been using this with AWS SDK Go code for a little while now and has been working nicely. No more trying to figure out the exact type. Rough usage would look something like this:

props := &BucketProps{
    Foo: jsii.Of("A string"),
    Bar: jsii.Of(123),
}

Also, an alternative to this is to provide setters so the setters deal with the references instead of the end user. Something like:

props := NewBucketProps().SetFoo("foo").SetBar(123)

// Or With
props2 := NewBucketProps().WithFoo("foo").WithBar(123)

Lastly, to help with required/vs optional props, perhaps solve that with a New function?

props := NewBucketProps("required1", "required2")
props.OptionalProp = "optional"
props.OptionalProp2 = 123

The UX isn't great though (cannot define inline), but combined with setters, maybe not as bad?

&SomethingThatNeedsABucket{
    Bucket: NewBucketProps("required1", "required2").SetFoo("foo").SetBar(123),
}

In the end, these are some random thoughts and I wanted to express them somewhere. 100% feel free to close 馃槃 馃憤

@polothy polothy added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 22, 2022
@MrArnoldPalmer
Copy link
Contributor

So we talked about something like the "setters" or builders basically for props objects but ultimately we weren't convinced that this was more idiomatic in Go land and felt like users would expect the current literal construction over using builders for props. However, props struct builders could likely be added without breakage if we did find an interface that we liked and users preferred.

The issue with a constructor function for props objects is that they have the same problem, all of the types for their input arguments would need to be pointer types, otherwise consumers would incur breaking changes when a source library went from having a required argument, to an optional one.

We explored using generics to wrap nullable altogether as well, see #3276 and aws/aws-cdk-rfcs#397 for details.

Since this issue was written we have added some solutions to make optional/required a bit more obvious (mainly in doc strings so IDEs can show this information) so check it out again and see what you think. I'll close this for now but if you do feel strongly regarding alternatives related to pointer types and optionality, feel free to reopen. Do check out the language bindings rfc PR and related discussion if you haven't already.

@github-actions
Copy link
Contributor

鈿狅笍COMMENT VISIBILITY WARNING鈿狅笍

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants