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

[Themes] Theme Console - Handle the REPL lifecycle #4224

Merged
merged 14 commits into from
Aug 6, 2024

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Jul 18, 2024

WHY are these changes introduced?

WHAT is this pull request doing?

EDIT - _This has been modified to use readline to accept console inputs _
https://github.com/user-attachments/assets/0645b141-be79-4c82-aff9-84797e1fb9fb

How to test your changes?

  1. Checkout the branch
  2. pnpm shopify theme console --dev-preview
  3. Play around with the console. Note that the error messages will change as the evaluator is implemented. (There are also different layers - the shutdown message, which stops the loop (hard to trigger) and the graceful message which catches the error and keeps the loop going.

Some ideas

  • Delimiter - {{ collections.first }} or {% x %}
  • Null value - f
  • Something that will error out - JSON(
  • Please feel free to suggest more cases!

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Jul 18, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.48% (+0.01% 🔼)
7815/10782
🟡 Branches
69.44% (+0.02% 🔼)
3849/5543
🟡 Functions 71.3% 2059/2888
🟡 Lines
72.78% (+0.01% 🔼)
7379/10139

Test suite run success

1788 tests passing in 813 suites.

Report generated by 🧪jest coverage report action from fe300f8

@jamesmengo jamesmengo force-pushed the jmeng/console-repl branch 3 times, most recently from 92c5ac5 to 7f102a7 Compare July 19, 2024 19:06
@jamesmengo jamesmengo changed the title Jmeng/console repl [Themes] Handle the REPL lifecycle Jul 19, 2024
@jamesmengo jamesmengo changed the title [Themes] Handle the REPL lifecycle [Themes] Theme Console - Handle the REPL lifecycle Jul 19, 2024
@jamesmengo jamesmengo force-pushed the jmeng/console-repl branch 2 times, most recently from e857760 to 124eb9d Compare July 19, 2024 19:58
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/content-tokens.d.ts
@@ -11,7 +11,7 @@ export declare class RawContentToken extends ContentToken<string> {
 export declare class LinkContentToken extends ContentToken<OutputMessage> {
     link: string;
     fallback: string | undefined;
-    constructor(value: OutputMessage, link?: string, fallback?: string);
+    constructor(value: OutputMessage, link: string, fallback?: string);
     output(): string;
 }
 export declare class CommandContentToken extends ContentToken<OutputMessage> {
packages/cli-kit/dist/public/node/output.d.ts
@@ -17,7 +17,7 @@ export declare const outputToken: {
     genericShellCommand(value: OutputMessage): CommandContentToken;
     json(value: unknown): JsonContentToken;
     path(value: OutputMessage): PathContentToken;
-    link(value: OutputMessage, link?: string, fallback?: string | undefined): LinkContentToken;
+    link(value: OutputMessage, link: string, fallback?: string | undefined): LinkContentToken;
     heading(value: OutputMessage): HeadingContentToken;
     subheading(value: OutputMessage): SubHeadingContentToken;
     italic(value: OutputMessage): ItalicContentToken;
packages/cli-kit/dist/public/node/path.d.ts
 /// <reference types="node" resolution-mode="require"/>
 import type { URL } from 'url';
 /**
  * Joins a list of paths together.
  *
  * @param paths - Paths to join.
  * @returns Joined path.
  */
 export declare function joinPath(...paths: string[]): string;
 /**
  * Normalizes a path.
  *
  * @param path - Path to normalize.
  * @returns Normalized path.
  */
 export declare function normalizePath(path: string): string;
 /**
  * Resolves a list of paths together.
  *
  * @param paths - Paths to resolve.
  * @returns Resolved path.
  */
 export declare function resolvePath(...paths: string[]): string;
 /**
  * Returns the relative path from one path to another.
  *
  * @param from - Path to resolve from.
  * @param to - Path to resolve to.
  * @returns Relative path.
  */
 export declare function relativePath(from: string, to: string): string;
 /**
  * Returns whether the path is absolute.
  *
  * @param path - Path to check.
  * @returns Whether the path is absolute.
  */
 export declare function isAbsolutePath(path: string): boolean;
 /**
  * Returns the directory name of a path.
  *
  * @param path - Path to get the directory name of.
  * @returns Directory name.
  */
 export declare function dirname(path: string): string;
 /**
  * Returns the base name of a path.
  *
  * @param path - Path to get the base name of.
  * @param ext - Optional extension to remove from the result.
  * @returns Base name.
  */
 export declare function basename(path: string, ext?: string): string;
 /**
  * Returns the extension of the path.
  *
  * @param path - Path to get the extension of.
  * @returns Extension.
  */
 export declare function extname(path: string): string;
 /**
  * Given an absolute filesystem path, it makes it relative to
  * the current working directory. This is useful when logging paths
  * to allow the users to click on the file and let the OS open it
  * in the editor of choice.
  *
  * @param path - Path to relativize.
  * @param dir - Current working directory.
  * @returns Relativized path.
  */
 export declare function relativizePath(path: string, dir?: string): string;
 /**
- * Given 2 paths, it returns whether the second path is a subpath of the first path.
- *
- * @param mainPath - The main path.
- * @param subpath - The subpath.
- * @returns Whether the subpath is a subpath of the main path.
- */
-export declare function isSubpath(mainPath: string, subpath: string): boolean;
-/**
  * Given a module's import.meta.url it returns the directory containing the module.
  *
  * @param moduleURL - The value of import.meta.url in the context of the caller module.
  * @returns The path to the directory containing the caller module.
  */
 export declare function moduleDirectory(moduleURL: string | URL): string;
 /**
  * When running a script using `npm run`, something interesting happens. If the current
  * folder does not have a `package.json` or a `node_modules` folder, npm will traverse
  * the directory tree upwards until it finds one. Then it will run the script and set
  * `process.cwd()` to that folder, while the actual path is stored in the INIT_CWD
  * environment variable (see here: https://docs.npmjs.com/cli/v9/commands/npm-run-script#description).
  *
  * @returns The path to the current working directory.
  */
 export declare function cwd(): string;
 /**
  * Tries to get the value of the `--path` argument, if provided.
  *
  * @param argv - The arguments to search for the `--path` argument.
  * @returns The value of the `--path` argument, if provided.
  */
 export declare function sniffForPath(argv?: string[]): string | undefined;
 /**
  * Returns whether the `--json` flag is present in the arguments.
  *
  * @param argv - The arguments to search for the `--json` flag.
  * @returns Whether the `--json` flag is present in the arguments.
  */
 export declare function sniffForJson(argv?: string[]): boolean;
packages/cli-kit/dist/public/node/ui.d.ts
@@ -19,7 +19,7 @@ import { Key as InkKey, RenderOptions } from 'ink';
 type PartialBy<T, TKey extends keyof T> = Omit<T, TKey> & Partial<Pick<T, TKey>>;
 interface UIDebugOptions {
     /** If true, don't check if the current terminal is interactive or not */
-    skipTTYCheck?: boolean;
+    skipTTYCheck: boolean;
 }
 export interface RenderConcurrentOptions extends PartialBy<ConcurrentOutputProps, 'abortSignal'> {
     renderOptions?: RenderOptions;
@@ -391,7 +391,7 @@ interface IsTTYOptions {
     stdin?: NodeJS.ReadStream;
     uiDebugOptions?: UIDebugOptions;
 }
-export declare function isTTY({ stdin, uiDebugOptions }?: IsTTYOptions): boolean;
+export declare function isTTY({ stdin, uiDebugOptions }: IsTTYOptions): boolean;
 export type Key = InkKey;
 export type InfoMessage = InfoMessageProps['message'];
 export { Task, TokenItem, InlineToken, LinkToken, TableColumn, InfoTableSection, ListToken, render, handleCtrlC };
\ No newline at end of file

@jamesmengo jamesmengo self-assigned this Jul 19, 2024
@jamesmengo jamesmengo added the #gsd:40767 Fortify local development experience for Liquid themes label Jul 19, 2024
@jamesmengo jamesmengo requested a review from karreiro July 19, 2024 20:01
@jamesmengo jamesmengo added the Area: @shopify/theme @shopify/theme package issues label Jul 19, 2024
@jamesmengo jamesmengo force-pushed the jmeng/console-repl branch 2 times, most recently from aef58f6 to f410f52 Compare July 23, 2024 21:21
@jamesmengo jamesmengo requested a review from karreiro July 23, 2024 21:21
@jamesmengo jamesmengo marked this pull request as ready for review July 23, 2024 21:22
Copy link
Contributor

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

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @jamesmengo! I've left only on minor comment, also should we rename the module to evaluator? Thank you!

}

function hasDelimiter(input: string): boolean {
return /\{\{|\}\}|\{%|%\}/.test(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches the Ruby implementation, but I wonder if we could be a bit more specific; something like this: /^\s*(\{\{|{%)/, this way we may avoid from failing in some specific valid use cases :)

@jamesmengo jamesmengo merged commit 465a3eb into jmeng/themeconsole Aug 6, 2024
@jamesmengo jamesmengo deleted the jmeng/console-repl branch August 6, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/theme @shopify/theme package issues #gsd:40767 Fortify local development experience for Liquid themes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants