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

[DNM] PR to look at -- Rough prototype for extension sourcemaps #4601

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shauns
Copy link
Contributor

@shauns shauns commented Oct 8, 2024

Opening PR for a place to put comments.

Copy link
Contributor

github-actions bot commented Oct 8, 2024

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/app-inner-loop

Copy link
Contributor

github-actions bot commented Oct 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.55% (-0.18% 🔻)
8520/11743
🟡 Branches
69.52% (-0.18% 🔻)
4177/6008
🟡 Functions
71.61% (-0.09% 🔻)
2205/3079
🟡 Lines
72.87% (-0.2% 🔻)
8062/11064
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / loader.ts
94.35% (-0.03% 🔻)
86.21% (+0.41% 🔼)
98% (-0.02% 🔻)
95.38% (-0.03% 🔻)
🟢
... / extension-instance.ts
80.69% (-4.71% 🔻)
73.83% (-1.41% 🔻)
91.3%
81.62% (-5.1% 🔻)
🟢
... / specification.ts
93.1% (-1.81% 🔻)
90.48%
87.5% (-0.5% 🔻)
92% (-2.12% 🔻)
🟢
... / context.ts
91.7% (-0.03% 🔻)
87.28% 88.57%
92.65% (-0.03% 🔻)
🔴
... / dev.ts
9.65% (-10.35% 🔻)
1.56% (-13.36% 🔻)
15.79% (-8.6% 🔻)
9.52% (-11.35% 🔻)
🟡
... / local-storage.ts
69.44% (-1.61% 🔻)
57.14% (-5.36% 🔻)
72.73%
67.65% (-1.8% 🔻)
🟢
... / link.ts
96.51% (-0.08% 🔻)
91.94% (-0.25% 🔻)
100%
96.39% (-0.09% 🔻)
🟡
... / update-extension.ts
64.86% (-1.8% 🔻)
54.55% 60%
68.75% (-2.22% 🔻)

Test suite run success

1938 tests passing in 873 suites.

Report generated by 🧪jest coverage report action from 0a0f80e

await this.build(options)

if (this.features.includes('bundling')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's use a new feature for this. You can provide it in extensions/specifications/ui_extension.ts and extensions/specifications/checkout_ui_extension.ts. 'generates_source_maps' or such. The reason is I'm not sure all bundling extensions would support this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the check for this feature should be happening much lower -- in the file below I think, when we're actually calling esbuild. Consult the feature to decide whether to make a source map or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of where to put the map file:

  • it should be the extensions' responsibility to deal with this
  • I think dist in the extension folder is fine -- it's usually gitignore-d. An alternate would be the .shopify folder in the root of the app, but I don't think that's necessary here

I would do the pulling out of the map file in the buildAndBundleExtensions function, prior to zipping:

//...
  showTimestamps: false,
    })

    await Promise.all(
      options.app.allExtensions.map((extension) => {
        extension.keepBuiltArtifactsLocally({bundleDirectory})
      }),
    )

    if (options.bundlePath) {
//...
    ```

You'll need to add `keepBuiltArtifactsLocally` to the extension interface. Default implementation should be a no-op. If an extension uses the `generates_source_map` feature, the behaviour should be to glob all the map files for this particular extension from `bundleDirectory` and move them to `joinPath(this.directory, 'dist', <whatever>)`

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes a lot of sense, thanks Shaun!

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.

2 participants