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

Commerce #38

Merged
merged 8 commits into from
Oct 2, 2024
Merged

Commerce #38

merged 8 commits into from
Oct 2, 2024

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Sep 26, 2024

No description provided.

@vicb vicb marked this pull request as draft September 26, 2024 11:55
@vicb vicb force-pushed the commerce branch 2 times, most recently from f37115f to 993aab7 Compare September 26, 2024 12:10
@vicb vicb marked this pull request as ready for review September 26, 2024 12:10
@dario-piotrowicz dario-piotrowicz force-pushed the commerce branch 4 times, most recently from 5450036 to 0d9bde4 Compare October 1, 2024 11:47
@vicb
Copy link
Contributor Author

vicb commented Oct 2, 2024

@dario-piotrowicz thanks for fixing the PR.

LGTM with a few comments:

  • I wouldn't update the app more than what is needed to run on workers
  • I wouldn't test things depending on the shop content that we don't control

@dario-piotrowicz
Copy link
Contributor

@vicb the tweaks I made to the app were actually necessary to get any e2e to work

so I've removed the e2es altogether... let me know what you prefer, we can either keep the e2es but re-add some slight tweaks to the app or have the app without any sort of e2e...

@vicb
Copy link
Contributor Author

vicb commented Oct 2, 2024

@dario-piotrowicz it's becoming hard to review, could you squash your commits in fewer commits to ease the review.

the tweaks I made to the app were actually necessary to get any e2e to work

I'm not sure to understand the root cause here.
If the CI creates the .dev.vars then it should be able to bring up the app?
But I guess it's not a big deal anyway.

@dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz it's becoming hard to review, could you squash your commits in fewer commits to ease the review.

sure

the tweaks I made to the app were actually necessary to get any e2e to work

I'm not sure to understand the root cause here.
If the CI creates the .dev.vars then it should be able to bring up the app?

I've remove that part, I think that we do not want the e2es to work only when the .dev.vars file is fully populated because this would be problematic for external contributors that would be forced to get a shopify key to get the e2es to work (adding a significant friction point in my opinion)

And the wrangler dev command without all the .dev.vars although works if you view the app with your browser is not getting detected as working properly by playwright (I suspect that playwright is getting some 500s when trying to see if the server is running)

@dario-piotrowicz
Copy link
Contributor

@vicb I've rebased and squashed my commits, please have another look 🙏

@vicb
Copy link
Contributor Author

vicb commented Oct 2, 2024

Looks good, thanks for making this work!

@vicb vicb merged commit 3628be6 into main Oct 2, 2024
2 checks passed
@vicb vicb deleted the commerce branch October 2, 2024 12:33
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.

2 participants