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

API conventions #55

Closed
stephenplusplus opened this issue Aug 28, 2015 · 3 comments
Closed

API conventions #55

stephenplusplus opened this issue Aug 28, 2015 · 3 comments
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@stephenplusplus
Copy link
Contributor

Oh boy, it's me again.

I think this library would be easier to fit into a modern Node developer's code if it was a bit easier to use.

Using the example from the readme:

(new GoogleAuth).getApplicationDefault(function(err, authClient) {

It's very rare you find the use of new Something() anymore. OO is still an important tool for structuring the code, but I think an approach like gcloud is better for the end user; basically, use OO internally, but don't require the user to see it. Alias by camelCase (as opposed to UpperCamelCase), and auto-initialize for them:

function GoogleAuth() {
  if (!(this instanceof GoogleAuth)) {
    return new GoogleAuth();
  }
}

module.exports.googleAuth = GoogleAuth;

// for the user:
googleAuth()
// is equivalent to:
new googleAuth()

  if (err === null) {
    // Inject scopes if they have not been injected by the environment
    if (authClient.createScopedRequired && authClient.createScopedRequired()) {

createScopedRequired being a method that may or may not exist is confusing. Can't this always be defined? Or maybe from the docs, it can be declared in what cases scopes are required up front?

** Edit ** Looking at this again, couldn't it just be a bool? If the function is sync, it should just be if (authClient.createScopedRequired) {}

      var scopes = [
        'https://www.googleapis.com/auth/cloud-platform',
        'https://www.googleapis.com/auth/compute'
      ];
      authClient = authClient.createScoped(scopes);
    }

The naming here is a bit confusing -- maybe it's just me. createScopedRequired and createScoped. Maybe this could be scopesRequired and addScopes.

Using addScopes would also avoid the re-assignment of the authClient. The pattern there feels a bit backwards. Get an authClient then get another one from it. It would be great if I could have gotten the authClient right from the start (as mentioned in the last suggestion with docs + requiring docs up front) or modify it so that it's good to go (addScopes).

  // Fetch the access token
    var _ = require(lodash);
    var optionalUri = null;  // optionally specify the URI being authorized
    var reqHeaders = {};
    authClient.getRequestMetadata(optionalUri, function(err, headers)) {
      if (err === null) {
        // Use authorization headers
        reqHeaders = _.merge(allHeaders, headers);
      }
    });

This is a good case for #54 -- if this library should be for the dev who just needs a token so they can talk to the API, it shouldn't be this hard. There should be a "getToken" method and a "extendRequestOptions" or something similar.


These are just some thoughts from the only code example given. I think docs and convenience methods will go a very long way, but in general, more terse naming conventions and a modern, intuitive API will help fill in the blanks.

Thanks for listening!

@ofrobots
Copy link
Contributor

The suggestions in this issue are reasonable, but given that they haven't been actioned in 2 years, I am inclined to say that this can be probably be closed now. @stephenplusplus any objections to closing this?

@JustinBeckwith
Copy link
Contributor

So... good news! The whole object instantiation thing in GoogleAuth case is now solved, and it's es modules compliant :) In the next release, you'll be able to do this:

const {auth} = require('google-auth-library');
auth.getApplicationDefault(...);

For the createScoped feedback - I don't entirely understand what's happening there, but I'm putting this up as a debate topic :) . Thoughts welcomed!

@JustinBeckwith
Copy link
Contributor

Since we addressed the first half of this, I went ahead and opened #264 to track the semver-major second part. Unless anyone objects, I'm closing this out.

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

5 participants