-
Notifications
You must be signed in to change notification settings - Fork 78
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
migrate gsheet api to public-google-sheets-parser to fix pfasLayer and spreadsheet utility #555
base: main
Are you sure you want to change the base?
Conversation
Some things I wanted to note.
|
Oof, will need to debug the tests too 👀 |
* migrate gsheet api to v4 * need of API_TOKEN * data reference not based of XML now direct json arrays
for pfas layer
a198ea7
to
a91125c
Compare
No need of API Token anymore :D and it works too. We still need the sheet to be public though. |
Will need to debug tests 😓 |
Just wondering if it's just not transpiled the right way! the new npm module I added. |
Yes, the error is due to public-google-sheets-parser not being transpiled right. I will have to look into how to configure that in the Gruntfile. After transpile
this is part of |
@jywarren any hints/suggestions for how to proceed with fixing these tests? |
Hmm, looking... could it be related to: |
src/pfasLayer.js
Outdated
@@ -1,156 +1,207 @@ | |||
const PublicGoogleSheetsParser = require("public-google-sheets-parser"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we require this instead, as it'll have been babelified and transpiled already?
https://github.com/fureweb-com/public-google-sheets-parser/blob/main/dist/index.js
So... maybe:
const PublicGoogleSheetsParser = require("public-google-sheets-parser"); | |
const PublicGoogleSheetsParser = require("node_modules/public-google-sheets-parser/dist/index.js"); |
It's worth a try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, i tried this but there was an issue which I filed a fix for in the upstream library: fureweb-com/public-google-sheets-parser#11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they don't merge it, we could change our package.json to point at: https://github.com/jywarren/public-google-sheets-parser/tree/patch-1
The transpile theory makes sense... Jasmine has trouble with some untranspiled code, i think i remember. Cypress shouldn't, but the errors there are different anyways:
|
Ugh - looks like I solved one issue but the original
|
Does that library by chance use async/await? https://stackoverflow.com/questions/33527653/babel-6-regeneratorruntime-is-not-defined says we need the |
Oh... Hi 👋🏽 I will go through the updates here. |
The failures right now are due to using a combination of Babel 6 ( the new deps ) and Babel 7 ( what we have already ) |
The failing cypress tests seems to be due to the new added test spreadsheet layer? Updating the count will do ig 🤔 If that is the case any new test will fail the test ig 👀 |
Updated the counts to make some progress. There are still errors related to undefineds, though -- is this ES6 incompatibility with Jasmine again? Then, i see a bunch related to missing images in the pfas layer test. Running it manually what do you think might be happening here?
|
Retriggering tests so we can see the latest logs |
v4public-google-sheets-parser
need of API_TOKEN * data reference not based of XML now direct json arraysFixes #553