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 error on shopify theme profile #5178

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

karreiro
Copy link
Contributor

Fix the speedscope error on shopify theme profile

Before:
image

After:
image

@karreiro karreiro requested a review from macournoyer January 10, 2025 13:24
@karreiro karreiro requested a review from a team as a code owner January 10, 2025 13:24
@karreiro karreiro mentioned this pull request Jan 10, 2025
10 tasks
Copy link
Contributor

github-actions bot commented Jan 10, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.14% (+0.01% 🔼)
8867/11800
🟡 Branches
70.33% (+0.02% 🔼)
4311/6130
🟡 Functions 75.06% 2318/3088
🟡 Lines
75.71% (+0.01% 🔼)
8383/11072

Test suite run success

2000 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 9b8226e

Copy link
Contributor

@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.

THANK YOU! 🙇

@karreiro karreiro merged commit 1e4e69c into theme-profile Jan 10, 2025
51 checks passed
@karreiro karreiro deleted the theme-profile-speedscope-fix branch January 10, 2025 14:31
@jamesmengo
Copy link
Contributor

jamesmengo commented Jan 10, 2025

I wanted to dig into why we needed to include speedscope again in the cli package. Leaving this note for future readers (I'm not too familiar with bundling so I may be off base - feel free to add corrections)

  1. In @shopify/theme:
"dependencies": {
  "speedscope": "1.21.0"  // Needed because theme uses speedscope directly
}
  • Theme package actually uses speedscope for profiling
  • It's marked as private: true, so its dependencies aren't visible to other packages
  1. In @shopify/cli:
"dependencies": {
  "speedscope": "1.21.0"  // Needed for Node 18 compatibility
}
  • CLI is the public package that users install
  • Needs speedscope for Node 18's require.resolve to work
  • Can't see theme's dependencies because it's private

The root cause is:

  1. We need to access speedscope's HTML file at runtime - here
  2. We support both Node 18 (CommonJS) and Node 20+ (ESM)
  3. CommonJS's require.resolve can't see dependencies of private packages
  4. Therefore, the public package needs its own copy of speedscope for Node 18 compatibility

This wouldn't be necessary if we either:

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

@macournoyer
Copy link
Contributor

Didn't need to access speedscope's files directly

That would make the profile feature not possible to implement, no?

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