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

Add some non gated app config specs #3170

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

alvaro-shopify
Copy link
Contributor

@alvaro-shopify alvaro-shopify commented Dec 5, 2023

WHY are these changes introduced?

Follow-up: #3161

NOTES:

  • Rebase follow-up branch before merging it
  • Edit this PR to use main before merging the follow-up

WHAT is this pull request doing?

  • Add some of the reaming configuration specs that are not gated (app access, declarative webhooks)
    • app_home
    • app_proxy
  • Add support to a declarative configuration to transforma the extension configuration content to send it/receive from the server. undefined fields won't be mapped
    • app_home spec format is different in the toml than in the backend
  • App loader only loads configuration extensions if they have some content (ie add a config section without any field inside because they are optional). This means that the extension wont be neither created in this case if it does not exists nor updated otherwise

How to test your changes?

  • Create a new cli app and run the app config link command to link it to a new remote app.
  • Edit the shopify.app.toml file and modify some value from:
    • application_url
    • embedded
    • app_preferences.url
    • app_proxy
  • Run the SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app deploy --path /PATH/TO/YOUR/APP command to release a new version.
  • Edit the shopify.app.toml file again modify some of the previous values
  • Run the npm run shopify app config link --path /PATH/TO/YOUR/APP command. The values should be overwritten with the released ones. This means that it's fetching the content from the extension registration, as we haven't run the command app config push to update the content

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

github-actions bot commented Dec 5, 2023

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.52% (+0.11% 🔼)
6376/8672
🟡 Branches
71.06% (+0.14% 🔼)
3101/4364
🟡 Functions
72.41% (+0.16% 🔼)
1630/2251
🟡 Lines
74.62% (+0.08% 🔼)
6040/8094
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / app_config_app_home.ts
100% 100% 100% 100%
🟢
... / app_config_app_proxy.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / loader.ts
92.88% (-0.23% 🔻)
85.38% (-1.12% 🔻)
95% (+0.13% 🔼)
93.89% (+0.09% 🔼)

Test suite run success

1527 tests passing in 690 suites.

Report generated by 🧪jest coverage report action from 82bdf70

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Nice work! Again, all the suggestions are minor and we can tackle them later if you prefer.

The only thing that didn't work as expected was the privacy_compliance webhooks, should it work? Is the server branch ready to handle it? I got all the fields after running link, except that one.

@alvaro-shopify alvaro-shopify changed the title Add remaining non gated app config specs Add some non gated app config specs Dec 20, 2023
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/constants.d.ts
@@ -28,6 +28,7 @@ export declare const environmentVariables: {
     identityToken: string;
     refreshToken: string;
     otelURL: string;
+    versionedAppConfig: string;
 };
 export declare const systemEnvironmentVariables: {
     backendPort: string;
packages/cli-kit/dist/public/common/object.d.ts
@@ -45,4 +45,21 @@ export declare function deepCompare(one: object, two: object): boolean;
  * @param two - The second object to be compared.
  * @returns Two objects containing the fields that are different, each one with the values of one object.
  */
-export declare function deepDifference(one: object, two: object): [object, object];
\ No newline at end of file
+export declare function deepDifference(one: object, two: object): [object, object];
+/**
+ * Gets the value at path of object. If the resolved value is undefined, the defaultValue is returned in its place.
+ *
+ * @param object - The object to query.
+ * @param path - The path of the property to get.
+ * @returns - Returns the resolved value.
+ */
+export declare function getPathValue<T = object>(object: object, path: string): T | undefined;
+/**
+ * Sets the value at path of object. If a portion of path doesn't exist, it's create.
+ *
+ * @param object - The object to modify.
+ * @param path - The path of the property to set.
+ * @param value - The value to set.
+ * @returns - Returns object.
+ */
+export declare function setPathValue(object: object, path: string, value?: unknown): object;
\ No newline at end of file
packages/cli-kit/dist/public/common/version.d.ts
@@ -1 +1 @@
-export declare const CLI_KIT_VERSION = "3.52.0";
\ No newline at end of file
+export declare const CLI_KIT_VERSION = "3.51.0";
\ No newline at end of file
packages/cli-kit/dist/public/node/context/local.d.ts
@@ -170,4 +170,11 @@ export declare function macAddress(): Promise<string>;
  * @returns The domain to send OTEL metrics to.
  */
 export declare function opentelemetryDomain(env?: NodeJS.ProcessEnv): string | undefined;
-export type CIMetadata = Metadata;
\ No newline at end of file
+export type CIMetadata = Metadata;
+/**
+ * Returns true if configuration extensions are enabled.
+ *
+ * @param env - Environment variables used when the cli is launched.
+ * @returns True if SHOPIFY_CLI_VERSIONED_APP_CONFIG is truthy.
+ */
+export declare function useVersionedAppConfig(env?: NodeJS.ProcessEnv): boolean;
\ No newline at end of file

Base automatically changed from add-support-app-config-spec to main December 21, 2023 09:24
@alvaro-shopify alvaro-shopify added this pull request to the merge queue Dec 21, 2023
Merged via the queue into main with commit 8d95a71 Dec 21, 2023
@alvaro-shopify alvaro-shopify deleted the add-remaining-app-config-specs branch December 21, 2023 12:46
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