search

njosefbeck / gatsby-source-stripe

Gatsby source plugin for building websites using Stripe as a data source
74 stars 17 forks source link

Improve file downloading #20 #21

Closed brxck closed 5 years ago

brxck commented 5 years ago

This should fix #20, so that all files should be finished downloading by the time sourceNodes() returns. Also createFileNode is now given the parent node id, in accordance with the changes to its API. In my brief testing this fixed some issues with cached nodes and the development server, as was suggested. Thanks @KyleAMathews for helping improve this!

Francesco-Lanciana commented 5 years ago

Hey @brxck @KyleAMathews as part of my PR for #22 I did a bit of refactoring, partly as I noticed that the promises for creating remote file nodes weren't quite being implemented correctly. This was primarily on the LocalFile file. I then realised that this PR touched the same file... I am completely happy to deal with the merge conflicts when this PR is accepted, however it might be worth having a look at my progress first.

I made some pretty significant changes to this file that I felt made it a lot more readable which if accepted would overwrite most of this PRs changes anyway. I haven't tested the changes just yet and I would really appreciate some input in case I missed anything, but I fully plan on having an actual PR for this tomorrow night.

Oh and I have two questions (I'll re-ask them on my issue as well to keep everything orderly)!

1) Do 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. I have changed it in my commit but I could very easily be overlooking something here.

2) 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?

Sorry for the super long comment, let me know what you think about my changes/proposal, again totally happy either way.

njosefbeck commented 5 years ago

I'm going to go ahead and merge this. Then, @brxck you can create a separate PR to implement Prettier/eslint. I'm fine with whatever code styles you choose. We'll probably need to add some information about running the linter to either the README, or the CONTRIBUTING file, or both.

@Francesco-Lanciana I'll respond to your issue here shortly.