Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Upgrade and tests #1

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

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

Upgrade and tests #1

thcolin wants to merge 4 commits into kevinschaich:master from thcolin:master

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 !

Copy link
Owner

kevinschaich commented May 6, 2017
edited
Loading

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.

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 and dmitry-yudakov reacted with thumbs up emoji

Copy link
Owner

kevinschaich commented May 7, 2017
edited
Loading

@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! 😄

Copy link
Author

thcolin commented May 10, 2017
edited
Loading

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

Copy link

drmercer commented Jul 10, 2017
edited
Loading

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.

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 ! 🙂

dmitry-yudakov reacted with thumbs up emoji

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

Reviewers

No reviews

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /