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

Fetch notifications in background #5020

Merged
merged 13 commits into from
Jan 8, 2025

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Dec 4, 2024

WHY are these changes introduced?

We are fetching the notifications at the beginning of each command and caching the result for 1 hour. That approach has two disadvantages:

  • When the network operation hangs or takes too long (the timeout is 3 seconds), the command has to wait with no feedback for the user (error example)
  • If we want to send an urgent notification (like a downtime), we have to wait for the local cache to expire (up to 1 hour)

WHAT is this pull request doing?

  • Fetch the notifications in a detached process that is run when the command finishes, so the users don't have to wait, and save the result in the cache
  • At the beginning of each command, we check the cache to show the required notifications
demo.mp4

How to test your changes?

Install the snapshot from this branch: npm i -g @shopify/cli@0.0.0-snapshot-20241216120051

  • shopify cache clear
  • shopify version => the notifications are downloaded when the command finishes
  • shopify version => notification should be shown
  • shopify version => no more notifications

The cache in Mac is stored here: ~/Library/Preferences/shopify-cli-kit-nodejs/config.json

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

@gonzaloriestra
Copy link
Contributor Author

/snapit

This comment was marked as outdated.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.19% (-0.17% 🔻)
8814/11723
🟡 Branches
70.46% (-0.1% 🔻)
4300/6103
🟡 Functions
74.98% (-0.26% 🔻)
2302/3070
🟡 Lines
75.76% (-0.13% 🔻)
8334/11000
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / identifiers.ts
96.23% (-3.77% 🔻)
89.19% (-1.29% 🔻)
100% 100%
🟡
... / context.ts
70.75%
60% (-2.07% 🔻)
73.91% 73.33%
🔴
... / extension.ts
53.85% (-1.71% 🔻)
58.33% (+0.64% 🔼)
50%
54.9% (-1.7% 🔻)
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% 90.48% 98.61%
🟢
... / dev-session.ts
85.96% (-1.03% 🔻)
68.33% (+1.09% 🔼)
91.67% (-0.93% 🔻)
92.93% (-0.4% 🔻)
🔴
... / app-management-client.ts
29.39% (-0.13% 🔻)
16.84% (-0.33% 🔻)
31.25%
28.22% (-0.18% 🔻)
🟡
... / api.ts
65.91% (-2.27% 🔻)
45.76% (-5.08% 🔻)
77.78% (-11.11% 🔻)
65.88% (-1.18% 🔻)
🟡
... / notifications-system.ts
63.96% (-2.7% 🔻)
62.35% (-3.32% 🔻)
85.71% (-3.76% 🔻)
71.43% (-2.9% 🔻)
🔴
... / system.ts
44.68% (-1.99% 🔻)
31.58% (+0.33% 🔼)
66.67%
45.65% (-2.08% 🔻)
🔴
... / graphql.ts
50% (-34% 🔻)
25% (-12.5% 🔻)
57.14% (-42.86% 🔻)
50% (-33.33% 🔻)
🔴
... / api.ts
55.48% (-3.82% 🔻)
44.44% (-0.68% 🔻)
56.41% (-3.59% 🔻)
56.55% (-4.18% 🔻)
🔴
... / factories.ts
33.33%
18.18% (-9.09% 🔻)
33.33% 41.67%

Test suite run success

1992 tests passing in 899 suites.

Report generated by 🧪jest coverage report action from 2375a3f

@gonzaloriestra gonzaloriestra marked this pull request as ready for review December 4, 2024 12:56
@gonzaloriestra gonzaloriestra requested review from a team as code owners December 4, 2024 12:56
@gonzaloriestra gonzaloriestra force-pushed the fetch-notifications-in-background branch from 8e9a3be to 7a1c90c Compare December 5, 2024 12:36
@gonzaloriestra
Copy link
Contributor Author

/snapit

This comment was marked as outdated.

@gonzaloriestra gonzaloriestra marked this pull request as draft December 5, 2024 15:25
@gonzaloriestra gonzaloriestra force-pushed the fetch-notifications-in-background branch from 38b5420 to 98e144d Compare December 16, 2024 10:38
@gonzaloriestra
Copy link
Contributor Author

/snapit

This comment was marked as outdated.

@gonzaloriestra
Copy link
Contributor Author

/snapit

Copy link
Contributor

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20241216120051

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@gonzaloriestra gonzaloriestra marked this pull request as ready for review December 16, 2024 15:31
Copy link
Contributor

github-actions bot commented Jan 7, 2025

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/public/node/notifications-system.d.ts
@@ -167,11 +167,25 @@ export type Notifications = zod.infer<typeof NotificationsSchema>;
  */
 export declare function showNotificationsIfNeeded(currentSurfaces?: string[], environment?: NodeJS.ProcessEnv): Promise<void>;
 /**
- * Get notifications list from cache (refreshed every hour) or fetch it if not present.
+ * Get notifications list from cache, that is updated in the background from bin/fetch-notifications.json.
  *
  * @returns A Notifications object.
  */
 export declare function getNotifications(): Promise<Notifications>;
+/**
+ * Fetch notifications from the CDN and chache them.
+ *
+ * @returns A string with the notifications.
+ */
+export declare function fetchNotifications(): Promise<Notifications>;
+/**
+ * Fetch notifications in background as a detached process.
+ *
+ * @param currentCommand - The current Shopify command being run.
+ * @param argv - The arguments passed to the current process.
+ * @param environment - Process environment variables.
+ */
+export declare function fetchNotificationsInBackground(currentCommand: string, argv?: string[], environment?: NodeJS.ProcessEnv): void;
 /**
  * Filters notifications based on the version of the CLI.
  *
packages/cli-kit/dist/public/node/system.d.ts
@@ -13,6 +13,7 @@ export interface ExecOptions {
     input?: string;
     signal?: AbortSignal;
     externalErrorHandler?: (error: unknown) => Promise<void>;
+    background?: boolean;
 }
 /**
  * Opens a URL in the user's default browser.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 9f9da19 Jan 8, 2025
27 checks passed
@gonzaloriestra gonzaloriestra deleted the fetch-notifications-in-background branch January 8, 2025 10:49
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