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

Re-org repo to prepare as a package #7

Merged
merged 10 commits into from
Nov 22, 2023
Merged

Re-org repo to prepare as a package #7

merged 10 commits into from
Nov 22, 2023

Conversation

markmur
Copy link
Contributor

@markmur markmur commented Nov 16, 2023

What are you trying to accomplish?

Divides the repo in 3 parts:

  • The base repo
  • A sample application
  • A Native module (which will be published to NPM)

@markmur markmur self-assigned this Nov 16, 2023
@markmur markmur force-pushed the package branch 3 times, most recently from 8606350 to a51984d Compare November 21, 2023 15:09
@@ -1,8 +0,0 @@
module.exports = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the root package.json to reduce the number of files

end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should hopefully help with our swiftlint woes

@@ -118,8 +118,6 @@ dependencies {
} else {
implementation jscFlavor
}

implementation("com.shopify:checkout-kit:${SHOPIFY_CHECKOUT_SDK_VERSION}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed since this now comes from the Native Modules (under modules/react-native-shopify-checkout-kit)

@@ -23,7 +23,6 @@ public boolean getUseDeveloperSupport() {
protected List<ReactPackage> getPackages() {
@SuppressWarnings("UnnecessaryLocalVariable")
List<ReactPackage> packages = new PackageList(this).getPackages();
packages.add(new ShopifyCheckoutPackage());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now auto-linked by React Native

sample/ios/Podfile Outdated Show resolved Hide resolved
@markmur markmur marked this pull request as ready for review November 22, 2023 10:48
@markmur markmur requested a review from a team as a code owner November 22, 2023 10:48
@caution-tape-bot
Copy link

👋 It looks like you're updating JavaScript packages that are known
to cause problems when duplicated.

You can deduplicate them with the yarn-deduplicate utility:

npx yarn-deduplicate --packages graphql react react-dom webpack
npx yarn-deduplicate --scopes @shopify @apollo

If running these commands doesn't produce a change in your yarn.lock file,
you didn't have duplications in these packages and can carry on.

A duplicate React version may cause an invalid hook call warning.

React context providers usually use module-scoped globals as their
default context values. Multiple versions of such packages will yield
multiple global instances, resulting in oblique runtime errors.


This comment was added by the YarnLock::DupeWarning Caution Tape Bot rule.
View the source of this rule in Services DB

@caution-tape-bot
Copy link

👋 It seems that this PR is adding a GitHub Action workflow that looks a lot like a CI step.

If your repository is a prodkit repository, then this is fine. Please ignore the rest of this comment.

If your repository already uses Shopify Build CI or Shipit CD, then please make sure to consider whether you are splitting your CI/CD pipelines and jobs in a way that makes sense and is easy to maintain.

If this workflow change is not introducing a CI step, then feel free to ignore this comment.

For more details and explanation, click to expand...

Shopify Build used to be the required platform for CI at Shopify, but we are now allowing GitHub Actions to be used for this purpose, especially if the repo is using ProdKit for deploys.

But it's not always a good idea to split your CI for a repo or service between different systems at once. We want to make sure that developers think about the potential maintenance costs of maintaining multiple systems before doing so.

For more information, see GitHub Actions on Vault.

Note that this message is based on a simple regex detection, and may not always be accurate. If your action is not a CI step, and is already running on shopify-ubuntu-latest or similar, feel free to ignore this message.

If your repository is intended to become Public later, please use runs-on: shopify-ubuntu-latest for now and ignore the rest of this comment. Though if you need to use Ubuntu 20.04 only, you would need to use runs-on: shopify-ubuntu-20.04. Or runs-on: shopify-ubuntu-latest-2 for Ubuntu 22.04.


This comment was added by the GitHubActions::CiStep Caution Tape Bot rule.
View the source of this rule in Services DB

@caution-tape-bot
Copy link

👋 It seems that this PR is adding, or editing, a GitHub Action workflow that is not configured to use our custom GitHub Action runners.

For private repositories, the only supported way of running Action workflows is via the custom runners. You can learn more about the supported use cases in our GitHub Actions documentation.

To make sure that your workflow is using these runners you need to add the shopify-ubuntu-latest label to your workflow manifest file, or shopify-core-ubuntu-latest for Core repositories:

runs-on: shopify-ubuntu-latest

If your repository is intended to become Public later, please use runs-on: shopify-ubuntu-latest for now. The custom runners should function the same as ubuntu-latest public runners, though the exact Ubuntu version may vary. Please see the section below for more information about OS selection on premium runners.

If you run into any issues with the custom runners, please reach out in help-eng-infrastructure on Discourse.

Note for OS Versions

The shopify-ubuntu-latest runners can select from two different runner pools, which contain both Ubuntu 20.04 and Ubuntu 22.04 machines. For some workflows, this can cause issues.

To use Ubuntu 20.04 only, you can specify runs-on: shopify-ubuntu-20.04.

To use Ubuntu 22.04 only, you can specify runs-on: shopify-ubuntu-latest-2.


This comment was added by the GitHubActions::Workflow Caution Tape Bot rule.
View the source of this rule in Services DB

@caution-tape-bot
Copy link

👋 This PR might modify or add cookies to Shopify, and these changes might be subject to privacy regulations. If your PR is not related to cookies, feel free to ignore this message.

Please avoid adding any new cookies to merchant storefronts unless absolutely necessary, we are undergoing an effort as a company to reduce our cookie footprint for legal reasons.

If you're adding/removing cookie(s) to storefronts, admin or checkouts, reach to us in #help-cookie-compliance so we can study your use case. After that, update this cookies list.

Instructions on updating the cookies list can be found here.

If your repository is not related to storefronts, and instead is related to other surfaces like shopify.com, shop.app, etc. our tooling can't support purpose-based blocking for you yet. This means that we would still like you to add the cookie to the allow list so we can keep track of it, but you should:

  • Choose strictly_necessary as the purpose
  • Explain in the description of the cookie that you chose strictly necessary because there is no visitor consent available in the client surface of your cookie.
  • Use ["other"] for the surface of the cookie.

If you have any other questions, reach out to #help-cookie-compliance in slack so we can help you.

Thanks!


This comment was added by the CookieCompliance::CookieBlaster Caution Tape Bot rule.
View the source of this rule in Services DB

@caution-tape-bot
Copy link

Shopify's style guide prefers avoiding terms like "white-list" and "black-list".

Drop in replacements include "allow-list" and "deny-list", or "clearlist" and "blocklist". Also consider if there is a domain specific term you could use.

For example, "parameter white-list" could be replaced with "parameter allow-list", but "trusted parameters" may be more suitable.


This comment was added by the BetterTerminology::Content Caution Tape Bot rule.
View the source of this rule in Services DB

.yarn/install-state.gz Outdated Show resolved Hide resolved
@markmur markmur merged commit 0cd0513 into main Nov 22, 2023
5 checks passed
@markmur markmur deleted the package branch November 22, 2023 12:10
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