Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate ShopifyAPI::Context in favour of instance-based configuration #1188

Closed
wants to merge 2 commits into from

Conversation

sterrym
Copy link
Contributor

@sterrym sterrym commented Jul 27, 2023

Description

This is an experiment to allow all methods that use ShopifyAPI::Context to use an instance-based configuration instead. There is an attempt to maintain backwards-compatibility as all method parameters still default to ShopifyAPI::Context

At time of writing, these are the impacted methods:

  • ShopifyAPI::Auth.embedded_app_url
  • ShopifyAPI::Auth::JwTPayload.new
  • ShopifyAPI::Auth::Oauth.begin_auth
  • ShopifyAPI::Auth::Oauth.validate_auth_callback
  • ShopifyAPI::Auth::Session.temp
  • ShopifyAPI::Clients::Graphql::Admin.new
  • ShopifyAPI::Clients::Graphql::Client.new
  • ShopifyAPI::Clients::HttpClient.new
  • ShopifyAPI::Clients::Rest::Admin.new
  • ShopifyAPI::Rest::Base.new
  • ShopifyAPI::Utils::GraphqlProxy.proxy_query
  • ShopifyAPI::Utils::HmacValidator.validate
  • ShopifyAPI::Utils::SessionUtils.current_session_id
  • ShopifyAPI::Webhooks::Registration.new
  • ShopifyAPI::Webhooks::Registrations::Http.new
  • ShopifyAPI::Webhooks::Registry.add_registration
  • ShopifyAPI::Webhooks::Registry.process
  • ShopifyAPI::Logger.debug
  • ShopifyAPI::Logger.info
  • ShopifyAPI::Logger.warn
  • ShopifyAPI::Logger.error
  • ShopifyAPI::Logger.deprecated

Please, include a summary of what the PR is for:

  • What is the problem it is solving?

There is a desire/need to create more than one Shopify App within a single Rails application.

  • What is the context of these changes?

  • What is the impact of this PR?

We are attempting to have zero-impact on current users other than potentially a deprecation notification.

How has this been tested?

All existing tests continue to function.
We added configuration parameter options to all tests as well.
We also top-hatted these changes by using this branch to create and install a new Shopify App

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

@sterrym sterrym changed the title Add config variable passed in to all methods/classes using Context Deprecate ShopifyAPI::Context in favour of instance-based configuration Jul 27, 2023
@lizkenyon
Copy link
Contributor

Hi there 👋

Could you give us a bit more context on the use case for this 👇

desire/need to create more than one Shopify App within a single Rails application.

@sterrym
Copy link
Contributor Author

sterrym commented Dec 8, 2023

Hi there 👋

Could you give us a bit more context on the use case for this 👇

desire/need to create more than one Shopify App within a single Rails application.

Absolutely. This was initially an internal need that we figured other partners would be interested in as well. Specifically, we are building multiple payments apps within an existing Shopify rails app. In our specific case, we are actually building a single Rails engine that supports three different types of payments apps. Each of them needs to be configured in a different way.

Internally, we ended up going another direction and are just using internal Shopify tools but I still suspect there's a valid use-case here.

@lizkenyon
Copy link
Contributor

Thanks @sterrym for the added context!

I am going to close this for now. But we may reach out for additional context if we want to move forward at a later date!

@lizkenyon lizkenyon closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants