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

Switch to verbObject() methods to .object().verb()? #862

Closed
jgeewax opened this issue Sep 14, 2015 · 15 comments
Closed

Switch to verbObject() methods to .object().verb()? #862

jgeewax opened this issue Sep 14, 2015 · 15 comments
Assignees
Labels

Comments

@jgeewax
Copy link
Contributor

jgeewax commented Sep 14, 2015

(Using createBucket as an example here)

If I want to "get a hold of a bucket", I can do it in two ways:

gcs.createBucket(name, callback) or gcs.bucket(name). But I have to know what the state of the world is. Get-or-create semantics don't exist, and if they did, it's two different method names, which is a little annoying.

How would we feel about switching to .object()-always syntax, which then has .verb() methods on it? That is, gcs.createBucket(name, callback) would become gcs.bucket(name).create(callback) ? And then for the get-or-create syntax, we could have that as a verb (.getOrCreate())?

So a bunch of examples:

var gcs = ...;
var bucket = gcs.bucket('my-bucket');
bucket.create(function(bucket, err) { ... });
bucket.getOrCreate(function(bucket, err) { ... });
bucket.loadMetadata(function(metadata, err) { ... });

Hypothetically, things like loadMetadata would update the instance of the bucket as well, right? That is, the bucket object in scope of the callback should be identical to the one outside of the scope because we'd store the metadata on this ?

@stephenplusplus
Copy link
Contributor

I've always argued against having a create method on an instance of a "thing," but I've heard this suggestion more than once, so maybe it's best I let go. My reasoning is just hierarchy:

Storage
-> Bucket
  -> Object
    -> Create Object...?

vs

Storage
-> Bucket
  -> Create Object
    -> Object

@callmehiphop for input on that. If it's what the people want, no argument here.

Similarly, I've argued that the autoCreate / getOrCreate idea would solve a problem that a bad architecture would have, so by supporting a solution, we're supporting that bad architecture. That always came up with PubSub / Topic. There are certain things that I think a developer has to solve before they fire up their app, or as a special "pre-boot" process. Using createBucket as an example: why would I need to get or create a bucket? Wouldn't it be more likely that I've previously created one for my specific use (say, "myapp-static-images", configured with a predefined public read ACL, etc). If I know the name of the bucket I want to use at run time, why don't I know that I already created it?

Additionally, things aren't always as simple as just "create a thing"; we support all of the options the API does for further customization of the new thing (createBucket(name, [options], callback) vs getOrCreate(name, [options], callback)-- optional params are certainly okay, but conditional params based on what the method ends up doing is really icky to me.

Bottom line, those are my reasons we haven't supported this stuff so far. This library is a collaboration of course, and it's just my opinion. I'll happily come up with the best solution I can if the team decides we need to support this.

@callmehiphop
Copy link
Contributor

From a hierarchy standpoint I think the verbObject() approach makes more sense. Although I have to admit when I was working on a spec for bigtable, at first I took the object.verb() approach because I was new to our way of doing things and to me it felt more intuitive.

Also, if I pretend I'm new to gcloud and I simply want to create a bucket, my initial reaction is to look at the bucket page which makes no mention to creating buckets.. bucket.create() would fix that -- assuming that's even an issue.

If our actual issue is that we just want to be able to get or create objects, instead of changing hierarchy maybe we should just modify the getter methods to do something like..

storage.bucket('my-bucket', { create: true }, function(bucket) {});

This would leave our API intact. We would just enhance the current getters to optionally accept a callback for an async version, which would allow us to create a bucket if it doesn't already exist.

@stephenplusplus
Copy link
Contributor

Also, if I pretend I'm new to gcloud and I simply want to create a bucket, my initial reaction is to look at the bucket page which makes no mention to creating buckets

👍 True, though we can also add something in the docs (maybe the Storage overview or the Bucket constructor).

instead of changing hierarchy, maybe we should just modify the getter methods to do something like..

storage.bucket() is not currently something that takes options or a callback, so that would change our API and (at least to me) make it confusing. The "autoCreate" (lazy create) arch makes a bit more sense to me:

var bucket = storage.bucket('my-bucket', { autoCreate: true });
var file = bucket.file('new-file');

// bucket didn't exist, so it made it and created file inside of it.
file.createWriteStream().end('hi!');

But I still have to wonder why I'm uploading files to a bucket that I don't know exists. What was my plan for after the file was uploaded? Does my app pull files from a bucket that I hope exists? Doesn't that seem like a poorly planned app? Maybe some use cases for this change would make it more clear to me and help us develop the ideal solution.

One last thing I forgot to mention in my first post for my argument against a object.create() method; it muddles what "object" is and does. For example, a Bucket instance always refers to a bucket that exists. It just keeps it easy.

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 16, 2015

though we can also add something in the docs

How scary would it be to alias this? ie, make bucket(name).create(cb) == createBucket(name, cb)? I know that now gives us two ways to do this, but it does solve one problem.

The "autoCreate" (lazy create) arch makes a bit more sense to me:

I agree that does solve the problem (and lazily so), but it means you're unable to execute a create action on the object explicitly (have to go up a layer in the hierarchy).


The confusion for our users is our mixture of the sync versus async. If the bucket exists, you do a var bucket = type sync operation. If the bucket doesn't, you do a method(function(bucket, ...)) async operation -- which means you actually have to delete code and re-write it (or wrap some if-statements with an existence check?). As far as I'm concerned, that's two ways to get a bucket -- with lots of deleted code depending on whether the bucket exists.

The goal of this issue (perhaps putting the carriage before the horse) was to find a way to pick one way (sync or async) that makes the getting-started case simple (and doesn't mess up the real-world use cases). Honestly, even doing storage.bucket(name, function(bucket, ...)) would be fine by me since it's all async style. But mixing and matching is making our users go "wait... huh?" when they're in the "I just want to try this shit out" stage.

I personally think that we should do:

  1. Getting a handle on an object should be sync-looking
  2. Doing any action on an object should be async-looking, and stem from the object itself (no deleting code or moving around in the hierarcy)

Or

  1. Always do async everything -- never return an object, always take a callback.

That said... I'm crazy, so ... tell me if this idea is ... dumb.

@stephenplusplus
Copy link
Contributor

async operation -- which means you actually have to delete code and re-write it (or wrap some if-statements with an existence check?)'

If you're not sure that a bucket exists, you can do:

var bucket = gcs.bucket('maybe-bucket');
bucket.getMetadata(function(err) {
  if (err) // you're not a thing, I need to create you
});

Is there a counter-argument to my point of why users are trying to use buckets that they don't know exist? Maybe for writing a tutorial/play app, but for those cases, it's kind of expected to have to be a bit wordy in your code, to compensate for the unknown-- but for your real app, the kind of thing we're writing this library for, I don't think you're dealing with the unknown.

But mixing and matching is making our users go "wait... huh?" when they're in the "I just want to try this shit out" stage.

This whole thing is starting to read to me as: "New users to your library don't know how to use it". That sounds to me like "We need better docs". I think we should add something like an "API Introduction" to the Getting Started section of our docs that goes over our conventions; creating objects, what the metadata property is on an object, how to reference an existing one.

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 20, 2015

If you're not sure that a bucket exists, you can do:

Yep - you totally can... but it's not intuitive. And it's become clear from UX studies that people don't read docs the way we expect them to... mostly they read enough until they think they recognize something, and apply whatever existing knowledge they have so far to that.

Maybe for writing a tutorial/play app

Absolutely that is the reason -- and this won't change for quite some time. We need to think of this as the entry-point to the library, so as long as it doesn't break things for more advanced users, it should be on the table.


So -- where do we take this convo next? I'm against solving this problem with docs, as the core problem is: people want to get started quickly, they see a bucket, they want to do stuff with it... by the time they get to looking at getMetadata, they've already figured out how all this stuff works anyway...

@stephenplusplus
Copy link
Contributor

So -- where do we take this convo next?

object.exists() will use getMetadata() under the hood and return true or false?

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 20, 2015

But doesn't that go further down the path of mixing async and sync-style calls? Or no?

@stephenplusplus
Copy link
Contributor

Are you saying users want this code?:

gcs.bucket('my-bucket', function(err, bucket) {
  // bucket was created or returned if it already existed
});

Okay, we can make that work. I do have my concerns:

  • We have to document the dual behavior of this method and explain why you want this one and not the other one
  • It's costlier; we have to make an API call to see if it exists or not. I used to think that wasn't a huge deal, but you can find repeated times in old issues and PRs from Googlers that it is

But I can't figure out how this should works for our APIs that require configuration:

compute.zone('us-centra1-a').vm('my-vm', function(err, vm) {}); // err! just a name isn't good enough.

To create a VM (and other methods in our library), you'll need to provide further configuration.. or simply when you want to provide further configuration.

compute.zone('us-centra1-a').vm('my-vm', { os: 'ubuntu' }, function(err, vm) {});

Wouldn't the last example be weird in practice? It's not just that the signature of the method is changing again, but having to specify your configuration every time you try to get or create a VM?

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 20, 2015

I think I'm saying users would want...:

gcs.bucket('my-bucket').getOrCreate(function(err, bucket) {
  // bucket was created or returned if it already existed
});

// or

bucket = gcs.bucket('my-bucket');
bucket.getOrCreate(function(err, bucket) {
  // bucket was created or returned if it already existed
});

// or 

gcs.bucket('my-bucket').get(function(err, bucket) {
// bucket must exist
});

For the compute example... (I'm totally pulling this out of thin air, so ... hang with me here)

var vm = compute.vm('name here', {whatever other options?});
vm.create(function(err, vm) {
 // your vm might have been created
});

// or

var zone = compute.zone('...');
vm.create({lots of other create-time-options ?, zone: zone}, function(err, vm) {
});

@stephenplusplus
Copy link
Contributor

Alrighty, if we always use getOrCreate:

var zone = compute.zone('us-central1-a');
var vm = zone.vm('my-vm');

vm.getOrCreate([cfg], function(err, createdVm) {
  // err = 'Missing Params' || null
  // createdVm === vm
});

Does this look okay?

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 20, 2015

And we also would have get and create ? right?

var zone = compute.zone('us-central1-a');
var vm = zone.vm('my-vm');

vm.get(vm, function(err, vm) {
  // shows all the metadata that comes with the VM? like attached devices, etc
});

vm.create([cfg], function(err, createdVm) {
  // fails if name already exists?
});

The goal that I had in mind was...

  • create an instance of a thing locally: sync (ie, var x = parent.x('required info');)
  • any verb stuff... async (ie, x.create([cfg], cb);)

@stephenplusplus
Copy link
Contributor

SGTM. Thanks for sticking with me, I think this solution will make everyone
happy :)

On Sunday, September 20, 2015, JJ Geewax notifications@github.com wrote:

And we also would have get and create ? right?

var zone = compute.zone('us-central1-a');var vm = zone.vm('my-vm');

vm.get(vm, function(err, vm) {
// shows all the metadata that comes with the VM? like attached devices, etc
});

vm.create([cfg], function(err, createdVm) {
// fails if name already exists?
});

The goal that I had in mind was...

  • create an instance of a thing locally: sync (ie, var x =
    parent.x('required info');)
  • any verb stuff... async (ie, x.create([cfg], cb);)


Reply to this email directly or view it on GitHub
#862 (comment)
.

@stephenplusplus
Copy link
Contributor

(Tagged the wrong issue in the PR. Sorry!)

@stephenplusplus
Copy link
Contributor

Fixed by #904.

sofisl pushed a commit that referenced this issue Nov 11, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [4.3.0](https://www.github.com/googleapis/nodejs-dialogflow/compare/v4.2.0...v4.3.0) (2021-08-16)


### Features

* expose `Locations` service to get/list avaliable locations of Dialogflow products; fixed some API annotations ([#860](https://www.github.com/googleapis/nodejs-dialogflow/issues/860)) ([05ea702](https://www.github.com/googleapis/nodejs-dialogflow/commit/05ea702372c08d0f0f157419799e59ea983df32a))


### Bug Fixes

* **build:** migrate to using main branch ([#862](https://www.github.com/googleapis/nodejs-dialogflow/issues/862)) ([ec0635c](https://www.github.com/googleapis/nodejs-dialogflow/commit/ec0635c17384c4fc5661d8a046b50b3fc2920505))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 17, 2022
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jsdoc](https://github.com/jsdoc/jsdoc) | [`^3.6.7` -> `^4.0.0`](https://renovatebot.com/diffs/npm/jsdoc/3.6.11/4.0.0) | [![age](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/compatibility-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/confidence-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-compute).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzQuMTkuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants