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

Support dotenv files #244

Merged
merged 6 commits into from
Nov 30, 2022
Merged

Support dotenv files #244

merged 6 commits into from
Nov 30, 2022

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Nov 25, 2022

Closes #195

Read from .env files and provide these variables in MiniOxygen's fetch handler. We might move this logic to MiniOxygen eventually.

Notes:

  • These variables can be accessed from the env object passed to MiniOxygen, not from process.env.
  • .env content does not override existing variables in process.env.

Questions:

  • Do we want to support .env.development, .env.local, etc? It might be useful in combination with the "env pull" feature we want to implement.
  • Do we want to support some kind of --env-path flag to specify the location of the .env file? -- There's already --path for the root project and we assume .env is there.
  • Should we filter variables from process.env by prefix? Right now we pass all the process variables to MiniOxygen. I think it doesn't really matter since this is only for local development, and prefixes would start making things confusing again.

cc @maxshirshin should we add this functionality directly to MiniOxygen? Or you think it's better in Hydrogen CLI?

@frandiox frandiox requested a review from a team November 25, 2022 04:57
@github-actions github-actions bot had a problem deploying to preview November 25, 2022 05:21 Failure
@frandiox
Copy link
Contributor Author

MiniOxygen just implemented dotenv as well so we can probably remove it from this PR.

Comment on lines +4 to +5
SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN="3b580e70970c4528da70c98e097c2fa0"
SHOPIFY_STORE_DOMAIN="hydrogen-preview"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have these variables already in the Hydrogen channel in admin. Are these variables automatically created for every Hydrogen channel? If that's true then we should keep the same name here. Otherwise we can simplify it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming that these are set up in the admin experience, so it's good that we match them here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we shouldn't be versioning .env - it should be gitignored. We will need to update our README.md to include the preview credentials, or perhaps a .env.example which is versioned and which does include preview credentials, so it's as easy as mv .env.example .env.

Copy link
Contributor Author

@frandiox frandiox Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .env is actually gitignored, I just force-committed it to have it working by default. It won't be committed after devs change its value so I think there shouldn't be any risk, right?

Otherwise, let me know if we still proceed with the .env.example and I'll change it in another PR 👍

(Plus, we need it for local development, so perhaps if we want .env.example we should rename it in the template creation script)

Comment on lines +97 to +100
clientOptions.storeDomain = clientOptions.storeDomain.replace(
'.myshopify.com',
'',
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this because the SHOPIFY_STORE_DOMAIN variable we have in Hydrogen channel includes .myshopify.com and I'm not sure if this variable is created automatically. If it's automatic, perhaps we should move this code to the h-ui utilities to make it more robust.
cc @frehner

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the place for this discush, but I do find it odd that our clientOptions doesn't accept a FQDN for the storeDomain argument. While most arguments will be *.myshopify.com domains, it's totally possible to use a custom domain as well - it's just up to the dev.

@frandiox
Copy link
Contributor Author

Next version of MiniOxygen (1.2.0) should also have this behavior, so it can probably be removed from this PR, unless we want to keep it to load env in Node (#245)

Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short of gitignoring .env, looks great!

Comment on lines +36 to +37
publicStorefrontToken: env.SHOPIFY_STOREFRONT_API_PUBLIC_TOKEN,
storeDomain: env.SHOPIFY_STORE_DOMAIN,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@frandiox frandiox merged commit 1db337a into main Nov 30, 2022
@frandiox frandiox deleted the fd-dotenv branch November 30, 2022 02:29
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.

Remove hardcoded storefront credentials
3 participants