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

Fix speedscope bundling #5214

Merged
merged 4 commits into from
Jan 16, 2025
Merged

Conversation

frandiox
Copy link
Contributor

WHY are these changes introduced?

Fixes the feedback in this comment for the shopify theme profile command.

WHAT is this pull request doing?

Moves speedscope from a dependency hard to bundle to the assets system we already have in place for Hydrogen and App.

How to test your changes?

Run pnpm bundle, then ls -la packages/cli/dist/assets/speedscope at the root and it should show all the files. Try the profile command to check that it works.

@frandiox frandiox requested review from a team as code owners January 16, 2025 14:40

This comment was marked as off-topic.

@frandiox frandiox mentioned this pull request Jan 16, 2025
10 tasks
@@ -24,7 +24,8 @@
".": {
"import": "./dist/index.js",
"types": "./dist/index.d.ts"
}
},
"./package.json": "./package.json"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this so that we can require.resolve('@shopify/cli/package.json') later.
If for some reason this is not desired, we can use findPathUp like we do in Hydrogen (we needed that in Hydrogen because the package can be run globally bundled, or locally, so require.resolve wouldn't be enough).

Comment on lines 6 to 9
export async function resolveAssetPath(...subpaths: string[]) {
const cliRootPath = dirname(require.resolve('@shopify/cli/package.json'))
return joinPath(cliRootPath, 'dist', 'assets', ...subpaths)
}
Copy link
Contributor Author

@frandiox frandiox Jan 16, 2025

Choose a reason for hiding this comment

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

Maybe this could be moved to cli-kit eventually? I see @shopify/app uses an ESBuild plugin to find the assets but perhaps this is simpler? @isaacroldan

Although it doesn't work directly on unit tests due to them running unbundled.

@frandiox frandiox force-pushed the fd-fix-speedscope-bundling branch from dc377b2 to f8bb4d6 Compare January 16, 2025 14:48
Copy link
Contributor

github-actions bot commented Jan 16, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 75.14% 8866/11800
🟡 Branches 70.31% 4310/6130
🟡 Functions 75.06% 2318/3088
🟡 Lines 75.7% 8382/11072

Test suite run success

2000 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from c3d1d92

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/public/node/themes/api.d.ts
@@ -22,5 +22,4 @@ export declare function metafieldDefinitionsByOwnerType(type: MetafieldOwnerType
         name: string;
         category: string;
     };
-}[]>;
-export declare function passwordProtected(session: AdminSession): Promise<boolean>;
\ No newline at end of file
+}[]>;
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/Alert.d.ts
@@ -1,10 +1,9 @@
 import { BannerType } from './Banner.js';
 import { BoldToken, InlineToken, LinkToken, TokenItem } from './TokenizedText.js';
-import { TabularDataProps } from './TabularData.js';
 import { FunctionComponent } from 'react';
 export interface CustomSection {
     title?: string;
-    body: TabularDataProps | TokenItem;
+    body: TokenItem;
 }
 export interface AlertProps {
     type: Exclude<BannerType, 'external_error'>;

Copy link

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

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

Awesome!! Thank you 🙇

@karreiro
Copy link
Contributor

/snapit

Copy link
Contributor

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

Test the snapshot by intalling your package globally:

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

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.

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.

Thanks a lot, @frandiox! 🚀 :) LGTM and works as expected as well 🎩

@macournoyer
Copy link

Merging into my branch now. Thanks!

@macournoyer macournoyer merged commit 12b9c49 into theme-profile Jan 16, 2025
27 checks passed
@macournoyer macournoyer deleted the fd-fix-speedscope-bundling branch January 16, 2025 17:45
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.

3 participants