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.

Sign up for GitHub

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

Jump to bottom

Update the Google Sheets API calls to v4 #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update the Google Sheets API calls to v4 #223

wants to merge 1 commit into from

Conversation

Lnaden
Copy link

@Lnaden Lnaden commented Aug 21, 2021 •
edited
Loading

This PR updates the API calls to Google Sheets to use v4 instead of v3
which was shut down August 2.

Fixes #217, Fixes #218, and Fixes #222 with some caveats, see below.

Full disclosure: I have no idea what I'm doing really with JS, so there
are likely to be improvements to be made.

Potentially Outstanding Issues:

  • Uses my personal API Key. Its restricted to Google Sheets API only,
    but I would prefer it be someone else's. Not really a problem right now.
  • Each Sheet is loaded in full through a GET request, returned entirely
    as JSON. This results in the load being a bit slow (and a fair amount
    of data comes down, the Official is 33MB of plain text returned for
    example. The load only happens once per sheet.
  • Can no longer get date-modified timestamp data from the Sheets API. You can from the Drive API but I don't think we want to give an API key more generic Drive API access. For now it just sets a date somewhere in the year 30000 to be "sometime newer than now" which should get cached with the right time on sheet load. Since the sheets are not updated anymore, probably not a huge issue.
  • I left in all the old code where I could in case improvements can be
    made.
  • I do not know JS, so any improvements, especially in the Promises
    area are likely very welcome.

Edits by maintainers are on and I'm happy to make any edits, but without explicit "do exactly this" I'll be fumbling around.

Edit: Forgot to add the time stamp note, since added

@Lnaden Lnaden mentioned this pull request Aug 21, 2021
Open
@Lnaden
Copy link
Author

Lnaden commented Aug 21, 2021

I tweaked my key somewhat to restrict it to localhost and kobold.club/fight/*. I'd like to see if it can be masked in its entirety, but in pure client side JS, I don't know how to do that. So, restricting to testing and the actual host site may have to do. A better solution will be needed here.

@Lnaden
Copy link
Author

Lnaden commented Aug 21, 2021

I had restricted the key to HTTP requests only on localhost and kobold.club, but that apparently locked it for testing. I removed that restriction and it should work now. I can fix the restrictions later or a better solution can be found.

Should work now from anywhere, but still restricted to the Google Sheets API

@williamgibb
Copy link

What is the environment you're using to build this project? I'd love to test this branch but it feels like there is a bit of a dependency / node issue going on (currently encounting issue with building node-gyp).

@Lnaden
Copy link
Author

Lnaden commented Aug 21, 2021

Nothing special near as I can tell.

  • npm installed via brew on OSX
  • Then just the instructions from the bottom of the README.md file
    • npm install for dependencies
    • npm run start to do testing on localhost:8080
    • npm run-script build to make the build/index.html and build/js/app-{hash}.js files.

I didn't do anything more than that and I haven't done any deployment, all local host. I can give you anything else you need/want

@Lnaden
Copy link
Author

Lnaden commented Aug 21, 2021 •
edited
Loading

A couple of other things I noticed on my own code:

  • Can no longer get date-modified timestamp data from the Sheets API, and I dont think we want to give an API key more generic Drive API access. Since the sheets are not updated anymore, probably not a huge issue. (This note added to the original post to keep it all in one place)

* I left a debug console.log command in. Will fix in a commit when I can get back to the machine this is on.

@Lnaden
Update the Google Sheets API calls to v4
8886bda 
This PR updates the API calls to Google Sheets to use v4 instead of v3
which was shut down August 2.

Fixes Asmor#217, Fixes Asmor#218, and Fixes Asmor#222 with some caveats, see below.

Full disclosure: I have no idea what I'm doing really with JS, so there
are likely to be improvements to be made.

Potentially Outstanding Issues:
* Uses my personal API Key. Its restricted to Google Sheets API only,
  but I would prefer it be someone else's. Not really a problem right now.
* Each Sheet is loaded in full through a GET request, returned entierly
  as JSON. This results in the load being a bit slow (and a fair amount
  of data comes down, the Official is 33MB of plain text returned for
  example. The load only happens once per sheet.
* I left in all the old code where I could in case improvements can be
  made.
* I do not know JS, so any improvements, especially in the Promises
  area are likely very welcome.

Removed some debug lines I left in sending to console.
@Lnaden
Copy link
Author

Lnaden commented Aug 22, 2021

Removed the debug lines I had left in, squashed the commit into the main one and force pushed. Sorry to those who had checked this out to test, however, there should be no functional difference for debugging issues.

@williamgibb
Copy link

What version of node is installed by brew? node --version should tell you.

@Lnaden
Copy link
Author

Lnaden commented Aug 22, 2021 •
edited
Loading

Node version 10.15.3. I haven't done a clean install in a while, so its whatever is on my machine here. I have tested this on a fresh ubuntu machine where I did an apt get install npm and it worked. I can get that version later if need be.

@williamgibb
Copy link

10.15.3 works; 12.x and 16.x don't work (node-sass isn't compatible).

@Lnaden
Copy link
Author

Lnaden commented Aug 22, 2021 •
edited
Loading

The package.json file does pin node-sass to >=3.11.1, <4.0.0, so I don't think this project even supports node versions that high. Although If I'm reading the dos right, we should move away from node-sass anyways

@Lnaden
Copy link
Author

Lnaden commented Aug 22, 2021 •
edited
Loading

I don't want to update the Node/node-sass in this PR anyways. I want this PR to do the bare minimum to get KFC working again without a huge security hole to maximize the chance it gets merged and deployed. @Asmor did say that PRs won't help (at least if its a rate limiting problem), so I wanted to make this as clean and straightforward a swap of the v3 -> v4 Sheets API that I could just in case it was a simple enough merge/deploy to keep the site going even a little longer.

@williamgibb
Copy link

williamgibb commented Aug 22, 2021 •
edited
Loading

Looking at modifying packages (and potentially needing to update for API changes and what not) should not be in scope for this delta.

@Lnaden
Copy link
Author

Lnaden commented Aug 22, 2021

I fully agree. Just making an admittedly needless/confusing observation, apologies.

@steverinoat
Copy link

hi all
I can't find the current state of the bug, but to let you know, site still does not work in chrome + chrome -incognito (cleared the browsing data). thx steve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Lnaden @williamgibb @steverinoat