Allow more control around auth headers for image downloads

[njosefbeck]

Allow more control around auth headers for image downloads

[njosefbeck]

This has now been merged, so I'm going to close this issue. Over the next couple of days I'll work on putting together an updated release for the project.

Thanks for your hard work and contribution! It's much appreciated :)

[Francesco-Lanciana]

@brxck @KyleAMathews Any chance you know why the localFiles___NODE property is put onto the different parts of the payload object and not just as a top level property?

This line specifically is very confusing:
const sourceObject = splitPath.length >= 1 ? this.getNestedObject(payload, splitPath.slice(0, -1)) : payload;

I would have thought you should just assign this once directly to the node object that is created by StripeObject.node(), this assigns it multiple times (for each filed) at different levels of the payload object.

[Francesco-Lanciana]

Another question! I was thinking that my PR should update the version to 3.0.0 (Major update) as there is a chance some people were relying on the current behaviour of the Authorization header when fetching images. My changes would make opting in to having an auth header explicit as having a default auth header may break things for people (me) but having no default header definitely won't (unless you were already relying on it due to the previous behaviour). Does this sound correct? Or am I wrong in thinking that no auth should be the default?

[njosefbeck]

Hi @Francesco-Lanciana, thanks so much for your contributions to this project! A couple of things to note:

1. I've merged @brxck's changes into master, so please make sure you pull updated master into your fork and refactor from there. Helping increase readability is great! Also, if you wouldn't mind adding comments to that file, so that others who come along can understand what's happening along the way, that'd be very helpful.

2. In your PR, make sure that you update the README with specific information about this new functionality around auth headers (i.e. what would the config now look like, what are the possible values that can be passed for auth, etc). Also please add your update to the CHANGELOG under the "Unreleased" section.

3. You're probably right that this re-work will mean a major release update. Once we look over your PR, and ultimately merge it into master, I'll handle updating the version as I'll also need to make sure the CHANGELOG properly reflects what has been changed.

LMK if you have any questions about contributing to the project, and thanks again 👍

[Francesco-Lanciana]

Hey @njosefbeck not a problem, glad to be working on it. As for the comments and README can do. I do have some questions though:

1. Any chance you know why the localFiles___NODE property is put onto the different parts of the payload object and not just as a top level property?

This line specifically is very confusing:
const sourceObject = splitPath.length >= 1 ? this.getNestedObject(payload, splitPath.slice(0, -1)) : payload;

I would have thought you should just assign this once directly to the node object that is created by StripeObject.node(), this assigns it multiple times (for each filed) at different levels of the payload object.

2. Why is it that we have a copy of the files in the src folder in the root of the project?

[njosefbeck]

Hey @Francesco-Lanciana!

I unfortunately can't answer your first question, as I'm not sure myself. Maybe @brxck has some insights.

As for your second question, the way the project currently works is:
(1) Make all changes inside /src/.
(2) Run npm run build to pass the src code through babel and output into the root dir.
(3) Then test the code

We do this for simplicity's sake, so that when someone installs our plugin, the code will work because it's been transpiled and moved to the root dir. I'd like to get around to refactoring this piece, so that it doesn't look like we have duplicate code, but I need to sift through some other gatsby plugins to see how they're doing it.

So, for further development at this time, don't worry about manually updating the files that are output in the root via src. Just run npm build to update those files with whatever changes you make in src and then do whatever testing you need to do. Apologies for the slightly annoying workflow!

[brxck]

Hey @Francesco-Lanciana thanks for working on this. Sorry for the opaque code, I appreciate the refactor. Let me see if I can help:

Any chance you know why the localFiles___NODE property is put onto the different parts of the payload object and not just as a top level property?

This is because of skus, specifically. A sku has an image field (sku.image) and a product field which has an images field (sku.product.images). These are separate image sets, which should not be combined and placed on the parent node. The line of code you are referring to places references to the downloaded file nodes on sku.localFiles & sku.product.localFiles.

[Francesco-Lanciana]

Hey thanks for the responses, I've readjusted the code to have multiple localFiles_NODE fields. I am however a little stuck. I'm testing that fetching images using the URL returned in the payload for file types work. For one of my files it does and for another it doesn't, returning the error Failed to process remote content. I've made sure that error doesn't break the plugin execution however I wanted to know if either of you have ever gotten this error? I'm passing all the same options to createRemoteFileNode as the current version so I'm hoping this is something we can skip over if it happens.

Other than that the last thing I'm unsure of is that in the current version of this plugin we iterate over payload.data in the downloadFileNode method, however that field doesn't exist on the payload that is returned back for me (also isn't in documentation). I just use the url field as suggested. I'm wondering how the current version ever worked or if file types were just broken til now but we didn't notice?

[Francesco-Lanciana]

Two more questions

1. Why is it that we give the parentNodeId: file.id option to createRemoteFileNode in the downloadFileNode method? I tried removing it and it had no effect on the result (I think).

2. Can I assume that file types will always have a payload with one URL (never multiple)? It seems to be a safe assumption from what I have examined from the payloads (and the fact the url field is a string in the Stripe documentation). Reason I ask is I can simplify the code somewhat if I can assume this.

[brxck]

I'm testing that fetching images using the URL returned in the payload for file types work. For one of my files it does and for another it doesn't, returning the error Failed to process remote content.

I think you are downloading from Stripe's File nodes, not Product/Sku, is that right? I don't recall ever seeing that error, but it looks like many things could cause it. It's hard to say without more testing.

in the current version of this plugin we iterate over payload.data in the downloadFileNode method, however that field doesn't exist on the payload

Since we don't have a test suite yet and I don't personally use this functionality, you probably found a regression. I believe that method used to consume a list of Files, but this has probably changed and you are now handling it correctly.

Why is it that we give the parentNodeId: file.id option to createRemoteFileNode in the downloadFileNode method?

See #20.

Can I assume that file types will always have a payload with one URL (never multiple)?

That's right, each Stripe File object represents a single file with a single url string.

[njosefbeck]

Just chiming in to answer your question here:

Other than that the last thing I'm unsure of is that in the current version of this plugin we iterate over payload.data in the downloadFileNode method, however that field doesn't exist on the payload that is returned back for me (also isn't in documentation).

For Stripe API calls that return lists, Stripe returns payloads that look like:

{
  "object": "list",
  "url": "/v1/products",
  "has_more": false,
  "data": [...]
}

Here's an example from their docs. So the payload originally does contain a data key. We use stripe-node's autopagination, which actually just returns each value inside of that data key, which is why you're not seeing a data key in your response.

So, indeed, this functionality could have been broken for a bit now. Definitely another reason to put together a solid suite of tests around this code. I'm going to plan to write those once we have your PR finalized and merged in.

[Francesco-Lanciana]

Thanks for the speedy responses. Has been really really helpful.

@brxck That's partly true, it actually just seems to be any file hosted on Stripe that is the problem, this is also the case for the SKU image (but not it's associated product images as they are hosted elsewhere). I agree though, will definitely need more testing, I'm a bit stumped as to why it's happening.

@njosefbeck Thanks for that, would have taken me quite some time to figure out why that was the case.