Notification system #4149

gonzaloriestra · 2024-07-03T07:33:15Z

WHY are these changes introduced?

We want to notify CLI users about deprecations, new features or events

WHAT is this pull request doing?

Adds two commands:

notifications generate: shows several prompts asking for the notification content, type and filters, and adds it to a local notifications.json file.
notifications list: shows the current notifications from the local notifications.json

Then, before each command we check the remote notifications.json from this repository and show the required ones. The file is cached for 24h.

Demo: https://hackdays.shopify.io/projects/18981

How to test your changes?

p shopify notifications generate
p shopify notifications list

Post-release steps

Merge the internal doc: https://github.com/Shopify/vault-pages/pull/7111

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

…into notifications-sytem

github-actions · 2024-07-03T07:33:33Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-management

gonzaloriestra · 2024-07-03T07:34:32Z

packages/cli-kit/src/public/node/notifications-system.ts

+import {CLI_KIT_VERSION} from '../common/version.js'
+import {NotificationsKey, cacheRetrieveOrRepopulate, getCache, setCache} from '../../private/node/conf-store.js'
+
+const URL = 'https://raw.githubusercontent.com/Shopify/cli/notifications/notifications.json'


We have to change this URL before merging to:

Suggested change

const URL = 'https://raw.githubusercontent.com/Shopify/cli/notifications/notifications.json'

const URL = 'https://raw.githubusercontent.com/Shopify/cli/main/notifications.json'

github-actions · 2024-07-03T07:37:08Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.4% (-0.16% 🔻)	7542/10417
🟡	Branches	68.99% (-0.21% 🔻)	3716/5386
🟡	Functions	71.61% (+0.14% 🔼)	1998/2790
🟡	Lines	72.79% (-0.11% 🔻)	7123/9786

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / generate.ts	0%	100%	0%	0%
🔴	`...` / list.ts	0%	100%	0%	0%
🔴	`...` / notifications.ts	0%	0%	0%	0%
🟢	`...` / global-context.ts	100%	100%	100%	100%
🟡	`...` / notifications-system.ts	64.47%	63.16%	83.33%	72.58%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / conf-store.ts	100%	70.59% (-6.33% 🔻)	100%	100%

Test suite run success

1735 tests passing in 797 suites.

Report generated by 🧪jest coverage report action from 3f53781

github-actions · 2024-07-03T08:18:25Z

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

packages/cli-kit/dist/public/node/global-context.d.ts

export interface GlobalContext {
    currentCommandId: string;
    [key: string]: string;
}
/**
 * Get the current command ID.
 *
 * @returns Current command ID.
 */
export declare function getCurrentCommandId(): string;
/**
 * Set the current command ID.
 *
 * @param commandId - Command ID.
 */
export declare function setCurrentCommandId(commandId: string): void;

packages/cli-kit/dist/public/node/notifications-system.d.ts

export interface Notifications {
    notifications: Notification[];
}
export interface Notification {
    id: string;
    message: string;
    cta?: {
        label: string;
        url: string;
    };
    type: 'info' | 'warning' | 'error';
    title?: string;
    minVersion?: string;
    maxVersion?: string;
    minDate?: string;
    maxDate?: string;
    commands?: string[];
    surface?: 'app' | 'theme' | 'hydrogen' | string;
    frequency?: 'always' | 'once' | 'once_a_day' | 'once_a_week';
}
/**
 * Shows notifications to the user if they meet the criteria specified in the notifications.json file.
 *
 * @param currentSurfaces - The surfaces present in the current project (usually for app extensions).
 * @returns - A promise that resolves when the notifications have been shown.
 */
export declare function showNotificationsIfNeeded(currentSurfaces?: string[]): Promise<void>;
/**
 * Get notifications list from cache or fetch it if not present.
 *
 * @returns A Notifications object.
 */
export declare function getNotifications(): Promise<Notifications>;
/**
 * Filters notifications based on the version of the CLI.
 *
 * @param notifications - The notifications to filter.
 * @param commandId - The command ID to filter by.
 * @param currentSurfaces - The surfaces present in the current project (usually for app extensions).
 * @param today - The current date.
 * @param currentVersion - The current version of the CLI.
 * @returns - The filtered notifications.
 */
export declare function filterNotifications(notifications: Notification[], commandId: string, currentSurfaces?: string[], today?: Date, currentVersion?: string): Notification[];
/**
 * Returns a string with the filters from a notification, one by line.
 *
 * @param notification - The notification to get the filters from.
 * @returns A string with human-readable filters from the notification.
 */
export declare function stringifyFilters(notification: Notification): string;
/**
 * Reads the notifications from the local file.
 *
 * @returns A Notifications object.
 */
export declare function getLocalNotifications(): Promise<Notifications>;

Existing type declarations

packages/cli-kit/dist/private/node/conf-store.d.ts

@@ -5,9 +5,13 @@ interface CacheValue<T> {
 }
 export type IntrospectionUrlKey = ;
 export type PackageVersionKey = ;
+export type NotificationsKey = ;
+type NotificationKey = ;
 interface Cache {
     [introspectionUrlKey: IntrospectionUrlKey]: CacheValue<string>;
     [packageVersionKey: PackageVersionKey]: CacheValue<string>;
+    [notifications: NotificationsKey]: CacheValue<string>;
+    [notification: NotificationKey]: CacheValue<string>;
 }
 export interface ConfSchema {
     sessionStore: string;
@@ -40,4 +44,6 @@ type CacheValueForKey<TKey extends keyof Cache> = NonNullable<Cache[TKey]>['valu
  * @returns The value from the cache or the result of the function.
  */
 export declare function cacheRetrieveOrRepopulate(key: keyof Cache, fn: () => Promise<CacheValueForKey<typeof key>>, timeout?: number, config?: LocalStorage<ConfSchema>): Promise<CacheValueForKey<typeof key>>;
+export declare function getCache(key: keyof Cache, config?: LocalStorage<ConfSchema>): CacheValue<string> | undefined;
+export declare function setCache(key: keyof Cache, value: string, config?: LocalStorage<ConfSchema>): void;
 export {};
\ No newline at end of file

github-actions · 2024-07-03T08:36:27Z

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.

isaacroldan · 2024-07-03T10:53:44Z

/snapit

shauns · 2024-07-04T11:37:44Z

packages/cli-kit/src/public/node/global-context.ts

@@ -0,0 +1,36 @@
+export interface GlobalContext {
+  currentCommandId: string
+  [key: string]: string


What's this for? It basically makes GlobalContext close to untyped -- I haven't seen a use of it. Would drop it if not used.

I added it initially to support anything, but we don't need it anymore if we have specific types for each property stored in the context. We should remove it.

shauns · 2024-07-04T11:42:40Z

packages/cli-kit/src/public/node/notifications-system.ts

+/**
+ * Fetch notifications from GitHub.
+ */
+async function fetchNotifications(): Promise<string> {


I think we should look into wrapping this in a timeout. Like, if it takes longer than Xms to load, we stop waiting and move on. Given that the command is paused whilst this is happening.

shauns · 2024-07-04T11:46:01Z

packages/cli-kit/src/public/node/notifications-system.ts

+  notifications: Notification[]
+}
+
+export interface Notification {


The typing here might not match the format used in the notification file -- e.g. if that file evolves but the CLI reading from it is an old one. I think it'd be helpful to defend against that -- whilst we're the authors of the notification file, we're still decoupled from it. I think I would swap this for a Zod schema, and parse the file using that. Then, if parsing fails because it doesn't match what this CLI's schema is expecting we can skip the notifications and move on. Obviously we would also try not to break the file as well.

That's a good idea 👌

We are catching errors here, so if the parsing fails, it already moves on. Any other advantage of using Zod here?

That only catches errors -- something that literally ends up in a thrown error -- if the format changes you might have a scenario where no error is thrown but the behaviour is wrong

shauns · 2024-07-04T11:49:20Z

packages/cli-kit/src/public/node/notifications-system.ts

+ * @param notifications - The notifications to render.
+ */
+async function renderNotifications(notifications: Notification[]) {
+  notifications.forEach((notification) => {


I think this is going to be quite noisy if there are multiple notifications matching on any given command run -- each section having it's own box. I'm not sure what the best alternative is. Or, how likely it is to happen. What do you think?

I don't know. We thought about it and decided to show all. What if there are two important notifications? Also, we are in control of the messages, so we should be careful about not bothering too much and try to minimize multiple ones.

However, we can improve it in the future and maybe add a priority field to only show one or two.

shauns · 2024-07-04T11:50:10Z

packages/cli-kit/src/public/node/notifications-system.ts

+ * Fetch notifications from GitHub.
+ */
+async function fetchNotifications(): Promise<string> {
+  const response = await fetch(URL)


There's an analytics helper to separate out network time from time with the command being active. We should wrap this up in that.

gonzaloriestra and others added 28 commits June 26, 2024 10:15

Sample json

4d32b20

Update notifications.json

f2a1ee5

Show notifications

58e7c09

Extract the notification system to its own file

29cbeb0

Fix date filter and render by type

92bf53b

Fix URL

e17405b

Update notifications.json

ed00213

Extract the filtering function

1d13c6d

Empty test

beb3870

add test for notification-system

1aac63e

Add cache

02db9ba

add more tests

45f23aa

Filter by surface

dd6b534

Filter by surface in app loader

03fa7ce

Surface filter for apps

8232c7d

Test with ui-extensino surface

8c9aa05

update notification json

4c0711a

update notification json

6f15859

Ignore commands when running notifications from loader

9d4ec89

Add more tests

7747888

Add filter by frequency

c952534

Merge branch 'notifications-sytem' of https://github.com/Shopify/cli …

7ffb885

…into notifications-sytem

Add support for CTA links

00ccbb1

new notification

f5a9704

List and generate commands

8eb6800

Delete notifications.json

017a901

Catch and ignore errors

b0160b8

Fix URL

4e10b20

gonzaloriestra added the includes-post-release-steps PRs including this label require additional steps after releasing label Jul 3, 2024

gonzaloriestra commented Jul 3, 2024

View reviewed changes

gonzaloriestra added 2 commits July 3, 2024 10:14

Reuse getCache and setCache

c490d08

Extract commands logic to a service

3f53781

gonzaloriestra marked this pull request as ready for review July 3, 2024 08:36

shauns reviewed Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification system #4149

Notification system #4149

gonzaloriestra commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024

gonzaloriestra Jul 3, 2024

github-actions bot commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

isaacroldan commented Jul 3, 2024

shauns Jul 4, 2024

isaacroldan Jul 4, 2024

shauns Jul 4, 2024

shauns Jul 4, 2024

isaacroldan Jul 4, 2024

gonzaloriestra Jul 4, 2024

shauns Jul 4, 2024

shauns Jul 4, 2024

gonzaloriestra Jul 4, 2024

shauns Jul 4, 2024

	const URL = 'https://raw.githubusercontent.com/Shopify/cli/notifications/notifications.json'
	const URL = 'https://raw.githubusercontent.com/Shopify/cli/main/notifications.json'

Notification system #4149

Are you sure you want to change the base?

Notification system #4149

Conversation

gonzaloriestra commented Jul 3, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

github-actions bot commented Jul 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2024 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Jul 3, 2024

Differences in type declarations

New type declarations

Existing type declarations

github-actions bot commented Jul 3, 2024

isaacroldan commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzaloriestra commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading