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

Upgrade and tests #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Upgrade and tests #1

wants to merge 4 commits into from

Conversation

thcolin
Copy link

@thcolin thcolin commented May 6, 2017

Hello !

I'm working on an application which use your library, and I got some troubles some weeks ago, the version of gapi.js you're using seems outdated, I got some error on load that I fixed by fetching new version at apis.google. The problem was it could happen again and I will be forced to upgrade version manually.

So I tough it would be a good idea to suggest my (first 🎉) pull request.

Here the changes I suggest :

  • Upgrade gapi.js version to f57af9a798d1e39a43f62aeefd014d5a (md5 checksum of gapi.js with module.exports, the index.jsin short, see next)
  • Creation of an upgrade.js (with an npm run upgrade shortcut) to upgrade script easily
    • It will fetch gapi.js from apis.google
    • Then add module.exports
    • And if md5 of this version is different from index.js, it will write it down
  • Unit tests implementation with mocha framework with an npm run test shortcut
    • Tests will check that gapi.js is loadable
    • I tried to check that version md5 is up to date too, but apis.google seems to return different versions depends on location, travis-ci servers got 272a52e59d1f98a4bf2409e77a029b62 when I got d3b6759a32936b3951340c194d448a70 or f57af9a798d1e39a43f62aeefd014d5a
  • I updated README by adding promisified example and API_KEY usage
  • Finally, I integrate Travis which can send mail when tests fail, and so be able to upgrade gapi.js version quickly !
    • It's possible to enable daily cronjob

Thanks !

@kevinschaich
Copy link
Owner

kevinschaich commented May 6, 2017

I definitely like the idea of having an "update" functionality for when the script gets out of date, however I think a better idea is to load the script dynamically on the client-side. I've been toying around with a few ideas over the past few weeks and I think something like this might be a nice solution:

import loadScript from 'load-script';
const GOOGLE_API_URL = 'https://apis.google.com/js/api.js';

/**
 *  Load the client library. Return a promise to allow .then() in caller
 */
load = (clientId, discoveryDocs, scope) => {
  return new Promise((resolve, reject) => {
    loadScript(GOOGLE_API_URL, () => {
      window.gapi.load('client:auth2', () => {
        window.gapi.client.init({
          discoveryDocs: discoveryDocs,
          clientId: clientId,
          scope: scope
        }).then(function () {
          resolve();
        });
      });
    });
  });
}

This way, we wouldn't have to release an update to the package every time Google updates the library as you mentioned above and there's a bit less moving parts. You'd use it in your app like this:

import load from 'gapi-client';

load(CLIENT_ID, DISCOVERY_DOCS, SCOPES).then((resp) => {
  // access api via window.gapi
  window.gapi.whatever()
});

which aligns nicely with some of the examples you added in the README.

One thing that would be AWESOME to have in future releases would be a way to specify additional client libraries (such as YouTube) that get loaded in a promise chain, and then resolve the .then() call when all libraries have successfully loaded.

@thcolin
Copy link
Author

thcolin commented May 6, 2017

Nice, better idea ! What about this ?

import loadScript from 'load-script'

var gapiLoader = function(){
  return new Promise(resolve => loadScript('https://apis.google.com/js/api.js', resolve))
}

With usages :

gapiLoader()
  .then(() => new Promise(resolve => gapi.load('client:auth2', resolve)))
  .then(() => gapi.client.init({
    discoveryDocs: ['https://www.googleapis.com/discovery/v1/apis/drive/v3/rest'],
    clientId: 'YOUR_CLIENT_ID',
    scope: 'https://www.googleapis.com/auth/drive.metadata.readonly'
  }))

gapiLoader()
  .then(() => new Promise(resolve => gapi.load('client', resolve)))
  .then(() => new Promise(resolve => gapi.client.load('youtube', 'v3', resolve)))
  .then(() => gapi.client.setApiKey('YOUR_API_KEY'))

Would be nice to avoid promisify gapi.load and gapi.client.load too !

@kevinschaich
Copy link
Owner

kevinschaich commented May 7, 2017

@thcolin I love that! Super clean implementation, and then users can specify in their app multiple libraries like client, YouTube, etc. As a quick note, you'll probably need to add load-script as a dependency in package.json.

Do you want to revise this/submit a new pull request (whatever is easier for you) with that implementation in index.js, changes in README.md, some tests, and Travis integration and then I'll approve it? Great work and thanks for the help! 😄

@thcolin
Copy link
Author

thcolin commented May 10, 2017

Yes, I will work on it this week, I think about using loadJS over load-script, which is promise based, to avoid to promisify loadScript, if you're okay with it ?

I'm thinking about promisify gapi too, to be consistent, but I'm still wondering how to do it and if the promise should return gapi or if we should keep it global :

// I thinks this solution would be more meaningful, don't you think ?
gapiLoader()
  .then(gapi => gapi.load('client:auth2', () => {}))

// This solution suggest `gapi` is global, it's not very explicit, if we go into this, I think we should properly document it
gapiLoader()
  .then(() => gapi.load('client:auth2', () => {}))

Sorry for late answers, quite busy at the moment !

PS : I will look at google/google-api-nodejs-client too, see if it can help us in any way

@drmercer
Copy link

drmercer commented Jul 10, 2017

Any progress on this? I'm getting 404 errors from client.load("client:auth2", ...) when trying to use gapi-client in my project, but not when I include the Google API script in my page directly. So this library is unfortunately unusable for me until this gets merged or the code gets upgraded.

Great idea to load the script on-the-fly, by the way. And a beautiful promise-y implementation, too. :)

Edit: Also, I'd be happy to help in any way I can with whatever needs doing to get this merged and published.

@thcolin
Copy link
Author

thcolin commented Jul 29, 2017

Hi !

Unfortunately, I didn't found any good solution to handle gapi promisify, so I finally create a gloader.js for my own needs, directly in my project, which look like these :

import loadJS from 'load-js'

const GOOGLE_API_URL = 'https://apis.google.com/js/api.js'

export default typeof gapi !== 'undefined' ? new Promise(gapi) : loadJS([GOOGLE_API_URL])
    // `gapi` is now available
    // WARNING ! App specific needs
    .then(() => new Promise(resolve => gapi.load('client', resolve)))
    .then(() => new Promise(resolve => gapi.client.load('youtube', 'v3', resolve)))
    .then(() => gapi)

The best, like we say earlier, would be to promisify directly in project all gapi methods, if you have any clue on how to do it, I'll be glad to see how ! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants