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

Standardize naming conventions of environment variables #1879

Closed
benjaminsehl opened this issue Jul 21, 2022 · 9 comments
Closed

Standardize naming conventions of environment variables #1879

benjaminsehl opened this issue Jul 21, 2022 · 9 comments
Assignees

Comments

@benjaminsehl
Copy link
Member

There are currently a few confusing problems with our environment variables today.

  1. Public vs. private both differentiate by the use of PUBLIC_ namespacing, as well as the use of Oxygen.env for private variables, unless you are not hosting with Oxygen — in which case you may either use cross-env (in Node) or in the case of Netlify, a plugin.
  2. SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN doesn't follow the same namespacing pattern, SHOPIFY_STORE_DOMAIN also doesn't follow the convention and also introduces STORE as a word (we now have Shop, Storefront, and Store all meaning the same thing)
  3. SHOPIFY_STOREFRONT_API_SECRET_TOKEN can’t be added to the Hydrogen config
  4. We don't provide a .env when a new repo is set up, so we also can't rely on it’s existence (today)

Proposal

  1. Switch to using Hydrogen.env as a consistent convention for accessing all environment variables, regardless of host, or whether private/public. If we can align on the import.meta convention with WinterCG, we will change over to that in the future.
  2. Rename Shopify provided variables to PUBLIC_STOREFRONT_API_TOKEN, PUBLIC_SHOPIFY_STOREFRONT_DOMAIN, STOREFRONT_API_SERVER_TOKEN (and enforce only PUBLIC_ can be exposed to the client, if not already)
  3. Make it possible to set the server token in the hydrogen.config file.
  4. We should create a env.example file that provides inline documentation linking to dev docs and shows a default configuration
  5. Update our documentation for Oxygen, Hydrogen, and for the steps required to make a server token in deployment outside of Oxygen
  6. Ideally, also introduce a method in the CLI to sync (pull/push) .env files, such that this can be automated

Caveat

Where above proposal creates a breaking change, we’ll hold off to group with other significant efforts coming to Hydrogen.

@frandiox
Copy link
Contributor

We've mentioned this before but here's an alternative:
Instead of relying on a global Hydrogen.env, we could instead import env or secrets from Hydrogen:

import secrets from '@shopify/hydrogen/secrets';

function App() {
  secrets.MY_SERVER_ONLY_VARIABLE;
}

The way we would polyfill this in different platforms is by using the already existing @shopify/hydrogen/platform utilities:

import {handleRequest, isAsset, setSecrets} from '@shopify/hydrogen/secrets';

export default {
  fetch(event, env, context) {
    if (isAsset(event)) ...;

    setSecrets(env);

    return handleRequest(event.request, {...});
  }
}

This way, we can ensure using the secrets is consistent in Hydrogen independently from the target platform. The only change is in the platform integration where they need to call setSecrets in one way or another. For example, in Node.js it would be setSecrets(process.env). Alternatively, it can be a parameter in handleRequest(req, {secrets: env}).

Instead of secrets it could be called env directly, but in this case I think we should provide access to import.meta.env as well from that object (e.g. env.PUBLIC_VAR).

Also, we could detect if @shopify/hydrogen/secrets is used in the browser build and throw errors.

@blittle
Copy link
Contributor

blittle commented Aug 11, 2022

I've been trying to get a handle on the environment variable problems brought up. I think this issue contains most of the reasoning that has brought us to where we are today. The key problem is that we are directly tied to Oxygen.env. We say:

By leaving server bundle runtime variables up to the hosting runtime (with a convenience mechanism built-in for Oxygen), Hydrogen doesn't need to be concerned with this.

But Hydrogen is inherently concerned with it because:

  1. We polyfill Oxygen in dev mode. So someone builds an app locally. Everything works fine. Then they deploy to Vercel, CF, Heroku Node, etc and now the world breaks until they define a global Oxygen variable? That is confusing. Why should I add an "Oxygen" variable to my Vercel environment?
  2. The s2s token is implicitly defined exclusively by an Oxygen environment variable. The only way to use the s2s token is to define an Oxygen environment variable in my Vercel, Node, etc environment. This is particularly bad because without the s2s token the merchant shop is almost certainly going to fail (rate limit) with any real world traffic.

To properly solve #1, I think there needs to be more discussion and alignment. At the very least we should immediately solve #2. I think we need to expose the s2s token in the Shopify config, and just have it point to an environment variable. This would remove the implicit dependency hardcoded in hydrogen, and make it obvious to the developer how to define it:

export default defineConfig({
  shopify: () => ({
    defaultCountryCode: 'US',
    defaultLanguageCode: 'EN',
    storeDomain:
      Oxygen?.env?.SHOPIFY_STORE_DOMAIN || 'hydrogen-preview.myshopify.com',
    storefrontToken:
      Oxygen?.env?.SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN ||
      '3b580e70970c4528da70c98e097c2fa0',
    storefrontSecretToken: Oxygen?.env?.OXYGEN_SECRET_TOKEN_ENVIRONMENT_VARIABLE,
    storefrontApiVersion: '2022-07',
  }),
})

For consistency, we should also rename them to:

  1. SHOPIFY_STORE_DOMAIN -> PUBLIC_SHOPIFY_STORE_DOMAIN
  2. SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN -> PUBLIC_SHOPIFY_STOREFRONT_API_TOKEN
  3. OXYGEN_SECRET_TOKEN_ENVIRONMENT_VARIABLE -> PRIVATE_SHOPIFY_STOREFRONT_API_TOKEN

We should also warn when PRIVATE_SHOPIFY_STOREFRONT_API_TOKEN is not defined and the app is running in prod mode. For backwards compatibility Oxygen should provide both values until we have a new major version release.

@frandiox
Copy link
Contributor

@blittle do you think we will have more secret variables in the shopify object at some point? We could do something like this:

shopify: () => ({
    defaultCountryCode: 'US',
    defaultLanguageCode: 'EN',
    storeDomain:
      Oxygen?.env?.SHOPIFY_STORE_DOMAIN || 'hydrogen-preview.myshopify.com',
    storefrontToken:
      Oxygen?.env?.SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN ||
      '3b580e70970c4528da70c98e097c2fa0',
    storefrontApiVersion: '2022-07',
    secrets: {
      storefrontServerToken: Oxygen?.env?.OXYGEN_SECRET_TOKEN_ENVIRONMENT_VARIABLE,
    }
})

@juanpprieto
Copy link
Contributor

Maybe multipassSecret could be another private variable

@ryanleichty
Copy link
Contributor

  1. We should create a env.example file that provides inline documentation linking to dev docs and shows a default configuration
  2. Update our documentation for Oxygen, Hydrogen, and for the steps required to make a server token in deployment outside of Oxygen

An example and additional documentation would be really helpful. I’m able to get my app running in dev mode with private variables, but I've been stuck trying to get the private variables to load in prod when deploying to Vercel.

@rennyG
Copy link
Contributor

rennyG commented Aug 15, 2022

Happy to work on the documentation. @ryanleichty or @blittle , could one of you spin up a documentation request with the details and assign it to me? If it'd be somebody else spinning up the issue, lmk who to reach out to.

@frehner
Copy link
Contributor

frehner commented Oct 5, 2022

Released in 1.4.0

@frehner frehner closed this as completed Oct 5, 2022
@JimLynchCodes
Copy link

Seems like these issues are still existing in a new scaffold. Jesus fucking christ can we please update these!!!

@JimLynchCodes
Copy link

"Admin API access token"
"API secret key"
"API key"

^ These are the ONLY credentials that exist in the shopify dashboard, and therefore are the ONLY names that should be used in the code.

Inventing your own names that don't match up with anything is completely fucking stupid.

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

No branches or pull requests

8 participants