Fix speedscope bundling #5214
Fix speedscope bundling #5214
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
| "types": "./dist/index.d.ts" | ||
| } | ||
| }, | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
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).
| export async function resolveAssetPath(...subpaths: string[]) { | ||
| const cliRootPath = dirname(require.resolve('@shopify/cli/package.json')) | ||
| return joinPath(cliRootPath, 'dist', 'assets', ...subpaths) | ||
| } |
There was a problem hiding this comment.
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
force-pushed
the
fd-fix-speedscope-bundling
branch
from
dc377b2 to
f8bb4d6
Compare
Coverage report
Test suite run success2000 tests passing in 904 suites. Report generated by 🧪jest coverage report action from c3d1d92 |
Differences in type declarationsWe 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:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/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'>;
|
macournoyer
left a comment
macournoyer
left a comment
There was a problem hiding this comment.
Awesome!! Thank you 🙇
|
/snapit |
|
🫰✨ Thanks @karreiro! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/[email protected]
|
|
Merging into my branch now. Thanks! |
3 participants
WHY are these changes introduced?
Fixes the feedback in this comment for the
shopify theme profilecommand.WHAT is this pull request doing?
Moves
speedscopefrom 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, thenls -la packages/cli/dist/assets/speedscopeat the root and it should show all the files. Try the profile command to check that it works.