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.

Sign up for GitHub

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

Jump to bottom

[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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion packages/app/src/cli/models/extensions/extension-instance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {hashString, randomUUID} from '@shopify/cli-kit/node/crypto'
import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn'
import {joinPath} from '@shopify/cli-kit/node/path'
import {useThemebundling} from '@shopify/cli-kit/node/context/local'
import {fileExists, touchFile, writeFile} from '@shopify/cli-kit/node/fs'
import {fileExists, moveFile, touchFile, writeFile} from '@shopify/cli-kit/node/fs'
import {getPathValue} from '@shopify/cli-kit/common/object'

export const CONFIG_EXTENSION_IDS = [
Expand Down Expand Up @@ -338,8 +338,20 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
this.outputPath = joinPath(bundleDirectory, extensionId, outputFile)
}

console.log('this.directory: ', this.directory)
console.log('this.outputPath: ', this.outputPath)

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!

// && if extension sourcemaps == true
const sourcemapOutputPath = joinPath(this.directory, 'dist', `${this.outputFileName}.map`)
console.log('sourcemapOutputPath ', sourcemapOutputPath)
const sourcemapInputPath = `${this.outputPath}.map`
moveFile(sourcemapInputPath, sourcemapOutputPath, {overwrite: true})
}

console.log('===')
if (this.isThemeExtension && useThemebundling()) {
await bundleThemeExtension(this, options)
}
Expand Down
1 change: 1 addition & 0 deletions packages/app/src/cli/services/build/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export async function buildUIExtension(extension: ExtensionInstance, options: Ex
env,
stderr: options.stderr,
stdout: options.stdout,
sourceMaps: true,
})
} catch (extensionBundlingError) {
// this fails if the app's own source code is broken; wrap such that this isn't flagged as a CLI bug
Expand Down
Loading