Skip to content

Threadsafe headers #95

Merged
merged 8 commits into from Feb 24, 2014

4 participants

@peterjm
peterjm commented Jan 14, 2014

Make ShopifyAPI compatible with threadsafe fork of ActiveResource.

There's enough demand for this (it's used in at least two Shopify apps, and there's an active forum thread about it) that I think it just makes sense to conditionally include it in core.

@phoet
phoet commented Jan 21, 2014

Is this connected to #52 ?

@peterjm
peterjm commented Jan 21, 2014

Yes. But this branch is dependent on my threadsafe fork of ActiveResource (https://github.com/peterjm/activeresource/tree/threadsafe), so we can't really merge this into master (unless I add a different version number to my AR branch, and we specifically check for it here in ShopifyAPI)

@kevinhughes27

this looks okay to me

@peterjm
peterjm commented Feb 19, 2014

If we do merge this, I think we should also cut a new gem version, so that we can stop telling people to use a github commit version entirely.

@maartenvg
Shopify member

Do you mean that we just release a new version and have them select your branch in their Gemfile? Or release a gem version that is threadsafe?

@peterjm
peterjm commented Feb 19, 2014

Yes, the idea would be that they could specify the threadsafe version of AR in their gem file, and if they're using an up-to-date version of the shopify_api gem then it will just pick it up.

@peterjm
peterjm commented Feb 19, 2014

Added a note to the README about thread safety.

@peterjm
peterjm commented Feb 21, 2014

Hey, just wanted to check in about this again.

@peterjm
peterjm commented Feb 21, 2014

We could (should) also move the AR branch to Shopify's control, so we're not recommending that people use some random person's (aka me) branch.

@maartenvg
Shopify member

Yeah, good idea. Let's do that first before we merge this.

@kevinhughes27

yup agreed

@maartenvg
Shopify member

Other than that it looks good to me. I haven't looked at your branch, but could we monkey patch instead of branching?

@peterjm
peterjm commented Feb 21, 2014

The branch re-writes significant parts of ActiveResource::Base. We could monkey patch, but it would end up being a really brittle monkey patch.

@maartenvg
Shopify member

cool, never mind then :)

@peterjm
peterjm commented Feb 21, 2014

I'm hoping that we'll eventually get the functionality back into AR core, anyway. They've started going down this road already.

@peterjm peterjm merged commit f23c14b into master Feb 24, 2014

1 check passed

Details default The Travis CI build passed
@peterjm peterjm deleted the threadsafe_headers branch Feb 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.