Download files feature refactor

[njosefbeck]

Hello!

As I dove into understanding how gatsby-source-filesystem worked, and how the download files feature worked, I thought I'd take a stab at refactoring the code to utilize my findings. Mainly I wanted to implement the Gatsby cache and write the code in such a way that it would be easy for me to write tests later.

Additionally, I wanted to make sure that we could 100% support the use cases of (1) downloading Product images, and (2) downloading a Sku image and any of its associated Product's images. Later on we can circle back and ensure that Files & File Links are supported as well.

Outside of writing tests for this code, which I will do before merging into master, I'd also like to make sure it's tested outside of the tests that I did. @brxck - if you wouldn't mind testing this PR, that'd be super helpful!

[brxck]

Sweet! I'll be sure to test it as soon as I can find the time!

[brxck]

Sorry for the accidental close! I'm on mobile...

[njosefbeck]

No worries :). And sounds good! I'm going to try to write all the tests tomorrow. I'll let you know when those are up on GitHub.

[brxck]

Finally got a chance to try testing this out! I've got two sites & Stripe accounts to test with, but so far I'm having no luck. The plugin does not create any Sku nodes: GraphQLError: Cannot query field "allStripeSku" on type "Query". Did you mean "allSite"?

I'll try to dig in more but May has not been kind to me so far, so no promises :sweat_smile:

[njosefbeck]

That's innnteresting. Do you get any errors in the console? Thanks for any and all testing you can do! It's been a busy month for me so far as well, so I totally understand!

[njosefbeck]

You also might try deleting your Gatsby cache and see if that changes anything.

[brxck]

No errors and tested after deleting the cache.

I do have two tests failing because of this, but idk if that's indicative of anything:

FAIL  src/__tests__/extractSkuUrls.test.js

  ● Test suite failed to run

    Cannot find module '../helpers/extractSkuUrls' from 'extractSkuUrls.test.js'

    > 1 | const extractSkuUrls = require("../helpers/extractSkuUrls");

[njosefbeck]

OK I think I've fixed that issue! You'll have to re-run npm run prepare once you pull down the update, but that file should be there now.

[njosefbeck]

@brxck Would you be able to test this once more this weekend by chance?

[brxck]

Hey sorry it's been so long! I just sat down to test this again. Last set of errors was because of the failing tests preventing the build completely. That's been resolved with your last commit.

One site works just fine now, the other, [https://github.com/brxck/gatsby-starter-stripe](the gatsby-starter-stripe demo), fails because the sku.product.localFiles field is not created. However, the plugin seems to download the images (hosted in the same GitHub repo) without issue. I haven't figured out why this is happening yet.

[njosefbeck]

Hmm, accessing localFiles off of sku product has worked for me in testing, but maybe there's some faulty logic in addLocalFiles.js. Whenever you have a chance to debug further, would be greatly appreciated!

Also, @Francesco-Lanciana if you have availability to test out this re-write, it'd be very helpful.

I'd like to get this into the plugin by the end of the month, if possible.

[Francesco-Lanciana]

Hey I have tested it against the site I'm currently building and it seems to be working almost fine (uses Sku, Product, both of which I use localFiles, and I also tested File). The two issues I ran into were that out of the 4 times I ran it 2 of those times it hung (the first time it ran, and when I added in File). Also it doesn't seem to log anything anymore (just blank lines). I'm a bit strapped for time at the moment so I might not be able to debug these issues but I hope that helps. Good work on the rewrite btw, it's looking good!

[njosefbeck]

Thanks for testing @Francesco-Lanciana! It's interesting that it's hanging. I actually haven't tested with File at all, as I just wanted to get Skus and Product images working first. I also haven't really implemented any logging, as Gatsby handles some of that for us. But maybe at this stage, to better figure out issues, I'll add some more robust logging.

I'll do some testing this weekend (hopefully), with @brxck's the gatsby-starter-stripe demo to try and figure out what might be erroring out there.

Thanks again to the both of you for your assistance with debugging! It's much appreciated 👍

[njosefbeck]

Hmmm, so I just tested gatsby-starter-stripe and everything worked fine for my Sku and Product images. I see in the filesystem and in the console that they've downloaded. I also see them under the localFiles field in GraphiQL. And the images I'm using are hosted in a variety of places: Netlify, Stripe, and GitHub pages. (I've also included a couple of images that won't resolve, to see how the plugin handles that.)

The console logs the progress of downloading the files, and logs when one errors out, though it doesn't provide much more extensive logging. Maybe that's necessary? Let me know.

Here's the GraphiQL output as well:

If we can do additional testing over the next two weeks to get to the bottom of the issues you both are experiencing around Skus and Products, that'd be fabulous. Otherwise I'll plan on merging this the weekend of June 7. Thanks again for all the testing help! It's much appreciated :heart:.

[brxck]

Sorry for the delay, been busy! Looks like the localFiles field on objects without images is now null rather than an empty array. Everything is working fine, but this change was causing issues with my implementation.

[njosefbeck]

Ah okay, so if I keep it null, I'll have to make a note that this could be a breaking change for people. Once you changed your implementation, does everything work now @brxck?

Thanks again for testing. It's always much appreciated :).

[brxck]

No problem! Yep, everything is working great after a couple small changes.

-------- Original Message -------- On May 31, 2019, 8:57 AM, Nathan Beck wrote:

Ah okay, so if I keep it null, I'll have to make a note that this could be a breaking change for people. Once you changed your implementation, does everything work now @brxck?

Thanks again for testing. It's always much appreciated :).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.