Skip to content

fix(extension-sdk): ensure asset imports work #1412

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

Merged
JammingBen merged 1 commit into from
Oct 22, 2025
Merged

fix(extension-sdk): ensure asset imports work #1412

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
43 changes: 38 additions & 5 deletions packages/extension-sdk/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,37 @@ const manifestPlugin = () => {
}
}

// UPSTREAM: https://github.com/vitejs/vite/pull/20861
// We are trying to upstream this plugin, it can be removed once the PR above or an alternative approach has been merged
// TL;DR of the issue: vite uses the require variable for certain features but doesn't ensure it's injected into AMD modules
// thus the global require variable is used which does not have the correct base path set, so relative imports
// with ?url or ?worker break
const completeAmdWrapPlugin = () => {
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

[nitpick] The regular expression is complex and lacks explanation. Consider adding a comment explaining what patterns it matches and why, especially the optional capture groups and spacing patterns.

Suggested change
const completeAmdWrapPlugin = () => {
const completeAmdWrapPlugin = () => {
// This regular expression matches AMD 'define' calls of the form:
// define([deps], function(params) { ... })
// or
// define(function(params) { ... })
//
// Breakdown:
// - \bdefine\( : matches the word 'define' followed by an opening parenthesis.
// - (?:\s*\[([^\]]*)\],)? : optionally matches a dependency array (e.g., [deps]), capturing the contents in group 1.
// - \s* : allows for optional whitespace before the array.
// - \[([^\]]*)\] : matches the array and captures its contents.
// - , : matches the comma after the array.
// - \s* : allows for optional whitespace after the dependency array or directly after '('.
// - (?:\(\s*)? : optionally matches an extra opening parenthesis and whitespace (for cases like define(([...]), ...)).
// - function\s*\(([^)]*)\)\s*\{ : matches 'function', optional whitespace, an opening parenthesis, captures the function parameters in group 2, closing parenthesis, optional whitespace, and opening curly brace.

Copilot uses AI. Check for mistakes.
const AmdWrapRE = /\bdefine\((?:\s*\[([^\]]*)\],)?\s*(?:\(\s*)?function\s*\(([^)]*)\)\s*\{/

return {
name: 'vite:force-amd-wrap-require',
enforce: 'pre',
renderChunk(code, _chunk, opts) {
if (opts.format !== 'amd') return

return {
code: code.replace(AmdWrapRE, (_, deps, params) => {
if (deps?.includes('"require"')) {
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

The check for \"require\" in the deps string is fragile. If deps contains 'require' (with single quotes) instead of \"require\" (with double quotes), this check will fail. Consider using a more robust pattern like /['"]require['"]/ to handle both quote styles.

Suggested change
if (deps?.includes('"require"')) {
if (deps && /['"]require['"]/.test(deps)) {

Copilot uses AI. Check for mistakes.
return `define([${deps}], (function(${params}) {`
}

const newDeps = deps ? `"require", ${deps}` : '"require"'
const newParams = params.trim() ? `require, ${params}` : 'require'

return `define([${newDeps}], (function(${newParams}) {`
}),
map: null // no need to generate sourcemap as no mapping exists for the wrapper
}
}
}
}

export const defineConfig = (overrides = {}) => {
return ({ mode }) => {
const isProduction = mode === 'production'
Expand All @@ -97,11 +128,7 @@ export const defineConfig = (overrides = {}) => {

return mergeConfig(
{
server: {
port,
strictPort: true,
...(isHttps && https)
},
base: './', // make asset paths relative so imports work from wherever the extension is loaded
build: {
cssCodeSplit: true,
minify: isProduction,
Expand Down Expand Up @@ -136,6 +163,7 @@ export const defineConfig = (overrides = {}) => {
}
},
plugins: [
completeAmdWrapPlugin(),
vue({
// set to true when switching to esm
customElement: false,
Expand All @@ -144,6 +172,11 @@ export const defineConfig = (overrides = {}) => {
manifestPlugin(),
tailwindcss()
],
server: {
port,
strictPort: true,
...(isHttps && https)
},
test: {
globals: true,
environment: 'happy-dom',
Expand Down