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

addition of --env flag to cli-hydrogen preview #239

Merged
merged 8 commits into from
Sep 16, 2022

Conversation

kcarra
Copy link
Contributor

@kcarra kcarra commented Aug 4, 2022

WHY are these changes introduced?

Currently when running shopify hydrogen preview the .env file is not included in the miniflare worker env and Oxygen.env never gets populated with those bindings. This pr adds the --env flag to specify a .env file to be used when creating the preview Oxygen environment. Examples of use:

shopify hydrogen preview --env=.env

shopify hydrogen preview --env=.preview.env

Fixes Shopify/hydrogen/issues/1901

WHAT is this pull request doing?

This pr adds the --env flag to the shopify hydrogen preview command to populate miniflare bindings

How to test your changes?

Vitest:
yarn test packages/cli-hydrogen/src/cli/services/preview.test.ts

yarn link:

  • checkout branch
  • cd into packages/cli-hydrogen
  • yarn yarn link
  • cd into a hydrogen project
  • yarn link yarn link "@shopify/cli-hydrogen"
  • run preview with .env file shopify hydrogen preview --env=.env
  • Verify that Oxygen.env is populated with values from .env file

@kcarra kcarra marked this pull request as ready for review August 4, 2022 13:41
Copy link
Contributor

@cartogram cartogram left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests! Can we add a minor changeset for this?

@rennyG throwing this on your radar for documenting the new flag.

@rennyG
Copy link
Contributor

rennyG commented Aug 17, 2022

Thanks for adding tests! Can we add a minor changeset for this?

@rennyG throwing this on your radar for documenting the new flag.

Thanks Matt! I've spun up this issue to address.

@kcarra
Copy link
Contributor Author

kcarra commented Aug 17, 2022

Just signed the CLA as well if you want to re-run the job @cartogram

@rennyG
Copy link
Contributor

rennyG commented Aug 17, 2022

What' the delivery by date for the updated docs, @cartogram?

@kcarra kcarra requested review from a team as code owners August 18, 2022 13:25
@kcarra
Copy link
Contributor Author

kcarra commented Aug 18, 2022

Thanks for adding tests! Can we add a minor changeset for this?

@rennyG throwing this on your radar for documenting the new flag.

Changeset added

@rennyG
Copy link
Contributor

rennyG commented Aug 19, 2022

shopify.dev PR, cc. @cartogram

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

As @matteodepalo pointed out, I'd use the existing utility in @shopify/cli-kit to get the content of the .env file into a Javascript object.

@kcarra kcarra force-pushed the cli-hydrogen-preview-env-flag branch from 8a373fb to 940d948 Compare September 9, 2022 13:52
@kcarra
Copy link
Contributor Author

kcarra commented Sep 9, 2022

@matteodepalo @pepicrft @isaacroldan addressed your feedback. Only thing I'm not sure about is why the async version of file.write isn't working in that test. I figured I would toss the question back to you guys for any suggestions before I spend to much time trying to debug it.

@kcarra kcarra requested review from matteodepalo and pepicrft and removed request for matteodepalo September 9, 2022 14:25
@kcarra kcarra requested review from isaacroldan, pepicrft and matteodepalo and removed request for pepicrft, isaacroldan and matteodepalo September 9, 2022 14:25
@matteodepalo
Copy link
Contributor

matteodepalo commented Sep 12, 2022

@kcarra I think what is happening is that file.write is stubbed so when you try to use it, it doesn't actually do anything. Instead of checking that file.write has been called with certain params, I wonder if we can remove the stub at the top (write: vi.fn()) and replace those checks with actual file.exists and content checks? It would require us to refactor other tests as well, but all those files get deleted anyway because they're written in tmp directories.

You can also try to play with vi.restoreAllMocks at the beginning of the test, although that would restore other mocks as well so it might make other things not work.

@kcarra
Copy link
Contributor Author

kcarra commented Sep 12, 2022

@matteodepalo great ideas! check out 30322d1 vitest allows you to restore your mock to the actual implementation if you pass it as an arg to vi.fn(impl)

@matteodepalo
Copy link
Contributor

@kcarra nice! 👍 approved.

@kcarra
Copy link
Contributor Author

kcarra commented Sep 15, 2022

@blittle @matteodepalo Any idea when this could get merged in?

@matteodepalo matteodepalo merged commit 108946d into Shopify:main Sep 16, 2022
@matteodepalo
Copy link
Contributor

Merged! Apologies, I think we were waiting on each other to merge :)

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.

[BUG] Secret .env variables not accessible in local preview
6 participants