Do we want ServiceObject#get()?

[stephenplusplus]

@jganetsk has some concerns, pasted below:

I don't have a problem with get() in general. It just seems obvious to me that an API shouldn't have a getter that returns an object of the same type as this.

Can you point me at APIs that do this, in the wild? Like does the string class in any programming language you know have a method called get() that returns a value of string type? Usually get() methods exist on types that are boxes of some sort... and they return what's inside the box. I've never seen a get() that returns the box itself. Like get() on unique_ptr<T> returns a T*. It would make no sense for it to return a unique_ptr<T>. And I don't think Javascript is an exception here.

But maybe what I find obvious is not necessarily obvious.

- @jganetsk googleapis/nodejs-pubsub#248 (comment)

The get() method currently serves two purposes:

• Populate the metadata property on the ServiceObject instance
• Allow auto-create behavior when used with the get({ autoCreate: true }) option

References back to when this method was conceived:

• My thoughts from 2015, similar to @jganetsk
• The first mention of get() by @jgeewax

I share the view that ServiceObject.get() is backwards from our standard hierarchy, which is, Service -> ServiceObject. I'm not familiar with other patterns that may exist in other libraries or languages, although I don't think that's necessary. I'd be interested in exploring a better way to support that feature, however, I'm not excited to remove this 3 year old, all-API method.

@callmehiphop @JustinBeckwith @ofrobots Thoughts are very appreciated, and please feel free to tag others.

[carnesen]

Personally I usually wouldn't name a method .get unless it were synchronously getting an internal property of the object. That being said, I didn't find it particularly confusing when I learned that this .get does what it does. In the past I created my own lil Node.js wrapper around the AWS SDK, and my method names follow a very similar convention to those used here. Whether or not the name .get could be better, it seems to me the ship has sailed. Stability of the API is too important to sacrifice for something that could be (and is already IMO) solved by documentation.

[callmehiphop]

I think semantically ServiceObject.get() always felt a bit weird, however I think there is value in the functionality it offers. From what I've seen in other client libraries, most offer a reload() method for fetching/refreshing metadata, however I would argue that autoCreate is probably the more desired feature. What if we were to provide another method that lives on the ServiceObject's parent?

e.g.

const PubSub = require('@google-cloud/pubsub');
const pubsub = new PubSub();
const topic = pubsub.topic('my-topic');

const subscription = await topic.getSubscription('my-sub', { autoCreate: true });

[jganetsk]

@callmehiphop yes, that sounds like what I was thinking.

[callmehiphop]

I might have gone a little overboard, but taking this into consideration I put together a gist with some improvements that I think could be made in PubSub, and well, most of the Node.js clients.

https://gist.github.com/callmehiphop/d81491fa89b86f688f07d68f2ccd61cb

IMO this would be a pretty hefty change, but I think it would help drive closing some core issues

• promise array results google-cloud-node#2896
• Consistently handle gax call options across methods. google-cloud-node#2895
• Returning arrays for everything makes for awkward and confusing code google-cloud-node#2556

I'm curious on what everyone's thoughts are!

[stephenplusplus]

That's awesome! I have some thoughts, but it might be easier if it could be converted to a public Google Doc. Would you mind doing that?

[JustinBeckwith]

Adding in @bcoe because this is a really interesting discussion.

[bcoe]

👋 hey just catching up on this conversation.

I like @callmehiphop's suggestion, as much as possible I've tried to avoid having state in objects that are data wrappers -- better that you fetch a new version of the same subscription right? and then there's no concern that something out of band might have reloaded the contents of your subscription object.

My other thought, is that in general we should avoid the method get, since it's a keyword on JavaScript classes now; better that it's something explicit like getSubscription.

[fhinkel]

Greetings, we're closing this due to inactivity. Please let us know if the issue needs to be reopened.