Add `theme profile` command #5109

macournoyer · 2024-12-13T20:56:10Z

WHY are these changes introduced?

https://hackdays.shopify.io/projects/19234

We want to expose Liquid profiling via:

Speedscope, in the browser
In our Theme VSCode extensions

The new theme profile command will be called for both. The VSCode extensions will use the --json flag.

WHAT is this pull request doing?

Add a shopify theme profile --url command that calls SFR asking for profiling data (using the Accept header).

Left to do

Default command to opening Speedscope in a browser window. JSON only via a --json arg.
Add authentication via OAuth token. Same as the theme dev command.
Add tests.
Test e2e when https://github.com/Shopify/storefront-renderer/pull/32295 ships
Test on a product page when https://github.com/Shopify/storefront-renderer/pull/32334 ships

How to test your changes?

Run dev shopify theme profile --store [YOUR STORE DOMAIN] --url /. Should open a browser window with Speedscope in it.

Post-release steps

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

github-actions · 2024-12-13T21:00:17Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	75.25% (+0.01% 🔼)	8881/11802
🟡	Branches	70.44% (+0.03% 🔼)	4329/6146
🟡	Functions	75.06%	2326/3099
🟡	Lines	75.81% (+0.02% 🔼)	8395/11074

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app-event-watcher.ts	93.83% (-1.23% 🔻)	86.49%	90.48%	98.61%

Test suite run success

2002 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 88d83f7

packages/theme/src/cli/services/profile.ts

macournoyer · 2024-12-18T20:31:58Z

packages/theme/src/cli/services/profile.ts

+  if (import.meta.resolve) {
+    return import.meta.resolve('speedscope/dist/release/index.html')
+  } else {
+    try {
+      const speedscopePath = require.resolve('speedscope/package.json')
+      const speedscopeDir = speedscopePath.replace('/package.json', '')
+      return `file://${speedscopeDir}/dist/release/index.html`
+    } catch (error) {
+      throw new Error("Can't find Speedscope package")
+    }
+  }


Seems to work both in tests and with dev shopify. Is there some packaging magic that could make this not work?

That doesn't work w/ the packaged CLI. Reported by @karreiro:

I believe I found the fix for this issue. Here's the the PR:
#5178

packages/theme/src/cli/commands/theme/profile.ts

.changeset/curvy-moons-judge.md

jamesmengo

Very cool project! Excited about this change.

I've added a couple of suggestions around changeset management, 1 of which is blocking change.

I wasn't able to get my 🎩 working, but I'll chock it up to user error and try again tommorow

Are there any edge cases you're aware of that we may want to take note of in case we need to patch / support them in the future?

macournoyer · 2025-01-07T14:56:50Z

I wasn't able to get my 🎩 working, but I'll chock it up to user error and try again tommorow

Hum... I'm a little worried about that error. Can you try w/ the --json option?

EvilGenius13 · 2025-01-07T15:06:39Z

I wasn't able to get my 🎩 working, but I'll chock it up to user error and try again tommorow

Hum... I'm a little worried about that error. Can you try w/ the --json option?

I had tried to 🎩 as well and got the same error. The json output is {"error": "unauthorized", "error_description": "The client is not authorized to perform this request"}%

macournoyer · 2025-01-07T18:29:18Z

@EvilGenius13 what is your store?

EvilGenius13 · 2025-01-07T18:50:36Z

@EvilGenius13 what is your store?

teamtws.myshopify.com

`dev shopify theme profile --url https://coded-courses.myshopify.com/` Only opens a browser page for now, no auth.

Since it's used dynamically

And use import.meta.resolve to load Speedscope

And strengthen how we resolve Speedscope

Co-authored-by: James Meng <35415298+jamesmengo@users.noreply.github.com>

* Fix speedscope error on `shopify theme profile` * Add 'speedscope' to 'packages/cli' ignored dependencies

karreiro

Thank you, @macournoyer! Really amazing stuff 🔥🚀

karreiro · 2025-01-10T15:20:58Z

👋 @Shopify/app-inner-loop, could you please take a look at this? :)

packages/theme/src/cli/commands/theme/profile.ts

karreiro · 2025-01-15T10:05:56Z

@Shopify/app-inner-loop, could you please take a look at this?

isaacroldan · 2025-01-15T10:38:52Z

packages/cli/package.json

@@ -102,6 +102,7 @@
  },
  "dependencies": {
    "@ast-grep/napi": "0.33.0",
+    "speedscope": "1.21.0",


why is this dependency in both cli and theme package.json's? It should be enough with the theme one

Good question... not sure @karreiro if you had to add this to fix the packaging?

I did a short exploration into this when I had some free time.

I am relatively green to TS / JS so I won't purport to be an expert on the module system ( or have much knowledge here at all yet) - info here

Would definitely prefer not to have to udpate the cli package.json

@jamesmengo thanks for the exploration! I'm unclear what solution you're suggesting, out of those:

Only supported Node 20+ (could use import.meta.resolve)
Made @shopify/theme public (dependencies would be visible)
Didn't need to access speedscope's files directly

That's actually not the reason why you need it there for require.resolve to work.

We bundle dependencies, once bundled, you can't access specific files with require.resolve. By adding it as a dependency of CLI, you are basically telling the CLI not to bundle this package, which will affect the total size and speed of the CLI installation.

You need to solve this issue in the bundle script, manually copying the files you want to access with require.resolve. (cli/bin/bundle.js).

Sorry but adding dependencies to CLI directly is not allowed as it affects all users performance.

@isaacroldan thanks for the context!

I should add some lint rule so that teams are warned about this earlier in the dev process and not in the final review, sorry I didn't see it earlier 🙏

gonzaloriestra

This is so cool! Have you tested it in Windows/Linux?

packages/theme/src/cli/commands/theme/profile.ts

Co-authored-by: Gonzalo Riestra <gonzalo.riestra@shopify.com>

And refresh docs

isaacroldan

Adding this here to make it clear that changes are requested, can't add new dependencies to cli :)

frandiox · 2025-01-16T14:41:22Z

Hi 👋
I had some context about bundling in the CLI thanks to some stuff we did in Hydrogen. I'm attempting to fix Isaac's feedback in this PR: #5214

With that, we """bundle""" speedscope assets and can access them in prod.

Fix speedscope bundling

macournoyer · 2025-01-16T18:10:01Z

@isaacroldan dependency got removed by @frandiox in latest commits. All good?

macournoyer commented Dec 16, 2024

View reviewed changes

packages/theme/src/cli/services/profile.ts Outdated Show resolved Hide resolved

ianks reviewed Dec 17, 2024

View reviewed changes

packages/theme/src/cli/services/profile.ts Outdated Show resolved Hide resolved

macournoyer commented Dec 18, 2024

View reviewed changes

macournoyer marked this pull request as ready for review January 6, 2025 14:30

macournoyer requested review from a team as code owners January 6, 2025 14:30

This comment has been minimized.

Sign in to view

macournoyer requested a review from ianks January 6, 2025 14:31

jamesmengo reviewed Jan 6, 2025

View reviewed changes

packages/theme/src/cli/commands/theme/profile.ts Show resolved Hide resolved

jamesmengo reviewed Jan 6, 2025

View reviewed changes

.changeset/curvy-moons-judge.md Outdated Show resolved Hide resolved

jamesmengo reviewed Jan 6, 2025

View reviewed changes

.changeset/curvy-moons-judge.md Outdated Show resolved Hide resolved

jamesmengo reviewed Jan 6, 2025

View reviewed changes

macournoyer added 13 commits January 8, 2025 09:24

Setup theme profile command

bab02b0

`dev shopify theme profile --url https://coded-courses.myshopify.com/` Only opens a browser page for now, no auth.

Output profiling JSON data to stdout

037a0fa

Open theme profile in Speedscope by default

45bb9f4

Add token auth to theme profile command

1347120

Refresh manifests

dbbdcf6

pnpm update-docs

7559132

Update lock file

0ab1b0f

Add knip entry for speedscope

60fe55c

Since it's used dynamically

Add new generated doc files I missed

2495c6b

Switch to using mime type for Liquid profiling

b65e554

And use import.meta.resolve to load Speedscope

Add tests for theme profile command

e981668

And strengthen how we resolve Speedscope

Add more doc to theme profile

34a2184

Add more validation around theme profile command

2141fc6

macournoyer and others added 6 commits January 9, 2025 09:10

Add waiting rainbow to theme profile command

d33d13e

Co-authored-by: James Meng <35415298+jamesmengo@users.noreply.github.com>

Add imports for Task in theme profile

7d21e67

Don't use native node os and fs modules

6810b3b

Restructure theme profile test following Given/When/Then

e222c18

Lint

5e9ef0b

Try removing speedscope from ignoreDependencies

93d2018

macournoyer requested a review from karreiro January 9, 2025 15:09

madmath mentioned this pull request Jan 10, 2025

[vscode extension] Profile annotations and webview Shopify/theme-tools#697

Draft

2 tasks

Fix speedscope error on shopify theme profile (#5178)

1e4e69c

* Fix speedscope error on `shopify theme profile` * Add 'speedscope' to 'packages/cli' ignored dependencies

karreiro approved these changes Jan 10, 2025

View reviewed changes

jamesmengo reviewed Jan 13, 2025

View reviewed changes

packages/theme/src/cli/commands/theme/profile.ts Outdated Show resolved Hide resolved

Fix json flag polluting stdout in theme profile

dbfa8b9

isaacroldan reviewed Jan 15, 2025

View reviewed changes

gonzaloriestra reviewed Jan 15, 2025

View reviewed changes

packages/theme/src/cli/commands/theme/profile.ts Show resolved Hide resolved

packages/theme/src/cli/commands/theme/profile.ts Outdated Show resolved Hide resolved

macournoyer and others added 2 commits January 15, 2025 17:00

Add use for theme profile w/o url flag

25d4dd2

Co-authored-by: Gonzalo Riestra <gonzalo.riestra@shopify.com>

Add --environment flag to theme profile

6e9888c

And refresh docs

isaacroldan requested changes Jan 16, 2025

View reviewed changes

frandiox mentioned this pull request Jan 16, 2025

Fix speedscope bundling #5214

Merged

frandiox and others added 5 commits January 16, 2025 15:47

Move speedscore from deps to assets

f8bb4d6

Fix path in unbundled unit tests

c1062a9

Ignore cli dep in knip

8eb3b92

Regenerate types.d.ts

c3d1d92

Merge pull request #5214 from Shopify/fd-fix-speedscope-bundling

12b9c49

Fix speedscope bundling

macournoyer requested a review from isaacroldan January 16, 2025 18:09

frandiox added 2 commits January 16, 2025 19:34

Merge branch 'main' into theme-profile

82e8981

Update usage of isStorefrontPasswordProtected

88d83f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `theme profile` command #5109

Add `theme profile` command #5109

macournoyer commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

macournoyer Dec 18, 2024

macournoyer Jan 9, 2025

karreiro Jan 10, 2025

This comment has been minimized.

jamesmengo left a comment

macournoyer commented Jan 7, 2025

EvilGenius13 commented Jan 7, 2025

macournoyer commented Jan 7, 2025

EvilGenius13 commented Jan 7, 2025

karreiro left a comment

karreiro commented Jan 10, 2025

karreiro commented Jan 15, 2025

isaacroldan Jan 15, 2025

macournoyer Jan 15, 2025

jamesmengo Jan 15, 2025

macournoyer Jan 15, 2025

isaacroldan Jan 16, 2025 •

edited

Loading

jamesmengo Jan 16, 2025

isaacroldan Jan 16, 2025

gonzaloriestra left a comment

isaacroldan left a comment

frandiox commented Jan 16, 2025

macournoyer commented Jan 16, 2025

Add theme profile command #5109

Are you sure you want to change the base?

Add theme profile command #5109

Conversation

macournoyer commented Dec 13, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

Left to do

How to test your changes?

Post-release steps

Measuring impact

Checklist

github-actions bot commented Dec 13, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jamesmengo left a comment

Choose a reason for hiding this comment

macournoyer commented Jan 7, 2025

EvilGenius13 commented Jan 7, 2025

macournoyer commented Jan 7, 2025

EvilGenius13 commented Jan 7, 2025

karreiro left a comment

Choose a reason for hiding this comment

karreiro commented Jan 10, 2025

karreiro commented Jan 15, 2025

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

isaacroldan Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzaloriestra left a comment

Choose a reason for hiding this comment

isaacroldan left a comment

Choose a reason for hiding this comment

frandiox commented Jan 16, 2025

macournoyer commented Jan 16, 2025

Add `theme profile` command #5109

Add `theme profile` command #5109

macournoyer commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

isaacroldan Jan 16, 2025 •

edited

Loading