-
Notifications
You must be signed in to change notification settings - Fork 325
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
Comments
We've mentioned this before but here's an alternative: 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 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 Instead of Also, we could detect if |
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
But Hydrogen is inherently concerned with it because:
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:
We should also warn when |
@blittle do you think we will have more secret variables in the 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,
}
}) |
Maybe |
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. |
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. |
Released in 1.4.0 |
Seems like these issues are still existing in a new scaffold. Jesus fucking christ can we please update these!!! |
"Admin API access token" ^ 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. |
There are currently a few confusing problems with our environment variables today.
PUBLIC_
namespacing, as well as the use ofOxygen.env
for private variables, unless you are not hosting with Oxygen — in which case you may either usecross-env
(in Node) or in the case of Netlify, a plugin.SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN
doesn't follow the same namespacing pattern,SHOPIFY_STORE_DOMAIN
also doesn't follow the convention and also introducesSTORE
as a word (we now haveShop
,Storefront
, andStore
all meaning the same thing)SHOPIFY_STOREFRONT_API_SECRET_TOKEN
can’t be added to the Hydrogen config.env
when a new repo is set up, so we also can't rely on it’s existence (today)Proposal
Hydrogen.env
as a consistent convention for accessing all environment variables, regardless of host, or whether private/public. If we can align on theimport.meta
convention with WinterCG, we will change over to that in the future.PUBLIC_STOREFRONT_API_TOKEN
,PUBLIC_SHOPIFY_STOREFRONT_DOMAIN
,STOREFRONT_API_SERVER_TOKEN
(and enforce onlyPUBLIC_
can be exposed to the client, if not already)hydrogen.config
file.env.example
file that provides inline documentation linking to dev docs and shows a default configuration.env
files, such that this can be automatedCaveat
Where above proposal creates a breaking change, we’ll hold off to group with other significant efforts coming to Hydrogen.
The text was updated successfully, but these errors were encountered: