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.14%	8866/11800
🟡	Branches	70.31%	4310/6130
🟡	Functions	75.06%	2318/3088
🟡	Lines	75.7% (+0.01% 🔼)	8382/11072

Show files with reduced coverage 🔻

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

Test suite run success

2000 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 93d2018

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:

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>

macournoyer · 2025-01-08T18:13:52Z

Everything should be working now!

We're set to replace https://shopify.dev/docs/storefronts/themes/tools/theme-inspector w/ this!

EvilGenius13

I 🎩 again and it's working both by opening a new window with the speedscope results and also when using --json to get the output. Also checked with bad routes. Just leaving a super minor question.

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

package.json

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

jamesmengo

🎩 and code LGTM!

Thanks for the changes, and for building this!
I left a couple of small comments but nothing blocking.

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

jamesmengo · 2025-01-09T02:31:15Z

Would like to get @karreiro input in case there's anything here that would be hard to revert once it's rolled out, such as flag semantics

karreiro · 2025-01-09T12:42:08Z

/snapit

github-actions · 2025-01-09T12:44:02Z

🫰✨ 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-20250109124236

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.

karreiro

Thanks a lot for this PR, @macournoyer! 🔥

Having a profiler in the same chain of tools is an excellent addition for theme developers. I'm sure this will raise awareness for TTFB performance on storefronts 🚀

I've left only two minor comments. Please let me know if you prefer that I handle them.

Thanks again for this PR!

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

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

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 added 3 commits January 8, 2025 09:28

Fix error handling in theme profile command

91ff0c4

Adjust tests expectations

487ed2f

Switch theme profile to use DevServer rendering

a0955f7

macournoyer force-pushed the theme-profile branch from 8fa9f2f to a0955f7 Compare January 8, 2025 14:28

macournoyer and others added 7 commits January 8, 2025 10:39

Update theme profile tests

a5ceb0b

Ignore typescipt error for conditional in resolveSpeedscope

de4b963

Lint

71f2677

more descriptive changelogs

2d7ef73

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

Swtich to minor version bump

a49cf3e

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

Refresh OCLIF manifest

2241a17

Regen docs

1a56b88

macournoyer requested a review from jamesmengo January 8, 2025 18:12

EvilGenius13 approved these changes Jan 8, 2025

View reviewed changes

EvilGenius13 reviewed Jan 8, 2025

View reviewed changes

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

jamesmengo reviewed Jan 9, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

jamesmengo reviewed Jan 9, 2025

View reviewed changes

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

jamesmengo approved these changes Jan 9, 2025

View reviewed changes

jamesmengo reviewed Jan 9, 2025

View reviewed changes

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

karreiro reviewed Jan 9, 2025

View reviewed changes

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

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

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

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

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

macournoyer commented Jan 8, 2025

EvilGenius13 left a comment

jamesmengo left a comment

jamesmengo commented Jan 9, 2025

karreiro commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

karreiro left a comment

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

macournoyer Dec 18, 2024

Choose a reason for hiding this comment

macournoyer Jan 9, 2025

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

macournoyer commented Jan 8, 2025

EvilGenius13 left a comment

Choose a reason for hiding this comment

jamesmengo left a comment

Choose a reason for hiding this comment

jamesmengo commented Jan 9, 2025

karreiro commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

karreiro left a comment

Choose a reason for hiding this comment

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