Incorporates Shopify's API Leaky bucket algorithm for upload of files

[rmartin]

hey @mikenorthorp ,

Please find my attached implementation of this solution to resolve https://github.com/mikenorthorp/gulp-shopify-upload/issues/10 . The takes advantage of the burst bucket size and automatically uploads the first 40 files at once. Subsequent files are queued to deploy roughly 2 files per second for the duration of the deploy. While this could technically be further optimized to watch the header response for a 'refill' of the burst speed, this seems sufficient to resolve any deployment errors due to the rate limit.

In addition to this solution I have also updated README, package and added a few editor configs to allow for consistency for the spacing and validation on the project. Let me know if you have any concerns with those additions / updates.

If you have a chance, give the overall approach a look and let me know if you'd like to see anything adjusted prior to merging in.

Cheers, Roy

[mikenorthorp]

Give me a few days to look this over, very busy at work lately :P I appreciate the work you put in.

[rmartin]

No problem, thanks @mikenorthorp .

[mikenorthorp]

So I did a merge of this and it seemed to break uploading, I'm not sure how many cases you tested but by default I do not set a base path or anything like that and it didn't seem to be working. I'm going to hold off on this merge until I have more time to integrate it fully, another pull request I merged broke a bunch of stuff which I believe I've fixed now.

[rmartin]

hey @mikenorthorp ,

I did test this extensively prior to committing and we have been using it for the last 10+ days in production deploying without issue. Just in case I did a quick test and confirmed that the deploy worked as expected even without the basePath and I could see the changes within the theme.

Could it have been something with the merge that went wrong? If you checkout directly from my branch does that work? Let me know if you have any other use cases that I can use to reproduce that issue, otherwise i'll try to merge in your latest changes into my branch to further test.

Scenarios Tested: 1. Folders inline with the gulpfule, no options or theme return gulp.src('./+(assets|layout|snippets|templates)/*/') .pipe(gulpShopify('API KEY', 'Password', 'MYSITE.myshopify.com', null, {}));

2. Folders inline with the gulpfule, no options with theme return gulp.src('./+(assets|layout|snippets|templates)/*/') .pipe(gulpShopify('API KEY', 'Password', 'MYSITE.myshopify.com', 'THEME ID', {}));

3. Folders within compiled dist (how we use it) return gulp.src('dist/+(assets|layout|snippets|templates`)/*/') .pipe(gulpShopify('API KEY', 'Password', 'MYSITE.myshopify.com', 'THEME ID', { 'basePath': 'dist/' }));

Unit Tests In general it might be worthwhile investing in unit test for this project. I struggled with introducing a changes to the code without unit tests and a fair amount of manual testing. I think this could be done even with the deployment process by providing a key / password / site / theme id in the testing file and then allow the tests to upload some testing data to confirm the code works as expected.

Let me know if that would help in this situation and we can get started on that first. Otherwise, I believe this should be working as expected.

Cheers, Roy

[joeldrapper]

I've tested this too and I get this error again #13 . I think that's to do with the time it was forked though. It's just a regression which should be fixed when it's merged in.

[joeldrapper]

Any update on this? Is there anything I can do to help get this PR merged?

[mikenorthorp]

Sorry just been realyl busy and don't want to commit a broken plugin, if it can be tested on Mac, Linux, and OSX to make sure their are no issues, that would be good.

On Wed, Sep 30, 2015 at 11:12 AM, Joel Drapper notifications@github.com wrote:

Any update on this? Is there anything I can do to help get this PR merged?

— Reply to this email directly or view it on GitHub https://github.com/mikenorthorp/gulp-shopify-upload/pull/11#issuecomment-144422260 .

[joeldrapper]

@mikenorthorp I can test it on Mac. Is there a version that's been merged with the recent fixes made since this PR was branched off?

[rmartin]

HI @joeldrapper and @mikenorthorp ,

I'll work on this over the weekend. I think what happened is that there was a bug fix that happened in parallel with my fix that I didn't incorporate into my branch which might have caused the issue. It's still strange that I wasn't getting the error but I'll ire-ntegrate this and test again and provide my thoughts on my findings.

Ultimately it would be great to incorporate some unit testing to help ensure that this is working as expected and if we find additional issues to add test cases for those as well. This is a little bit of a hard one to test for as it's difficult to simulate the Shopify environment without hardcoding a key. If you have any thoughts about how to incorporate those tests let me know and i'll try to work on that as well.

Cheers, Roy

[joeldrapper]

@rmartin did you manage to get this re-integrated and tested? Happy to help with testing if you can update the PR.

[rmartin]

hey @joeldrapper and @mikenorthorp ,

I have incorporated the latest changes into my PR and tested the three available deployment scenarios with a project that has 100+ files to synchronize. I confirmed that with the existing code in trunk that the files hit the 'Error undefined! ShopifyCallLimitError' after 40 files. When testing again with this latest PR I see that this works as expected and deploys all 100+ files.

I'm still trying to puzzle through the best way to provide a unit test for this specific function to mock-up files and then deploy them. In the meantime, I don't want to hold up the fix and overall this should be working as expected now.

Please review and let me know if you run into any issues and I'd be happy to make any needed modification. Noting that I tested this on OSX 10.11 and an Ubuntu 14.04 trusty VM via vagrant.

Cheers, Roy

[joeldrapper]

@rmartin thank you. I'm going to test it now and I'll have a think about the unit tests too.

[mikenorthorp]

Thanks I've merged this and updated the package on NPM to release 2.0.0