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 import dev toolbar apps on windows #9400

Merged
merged 2 commits into from
Dec 18, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 11, 2023

Changes

fix #9389

Use JSON.stringify when injecting arbitrary paths so it works on Windows (backslashes wont cause problems)

Testing

I'm not sure if this will fix it, might need help testing from someone on Windows, or add a test (marking draft)

Docs

n/a. bug fix.

Copy link

changeset-bot bot commented Dec 11, 2023

🦋 Changeset detected

Latest commit: e62a6e9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 11, 2023
@66Leo66
Copy link
Contributor

66Leo66 commented Dec 15, 2023

@bluwy Hi! I'm the author of #9389 and I can confirm this fixes the problem (on astro's side) on my machine. This patch addressed the second error in the log of original issue. (I did a quick test via pnpm patch astro instead of building astro from scratch)

--- a/dist/vite-plugin-dev-overlay/vite-plugin-dev-overlay.js
+++ b/dist/vite-plugin-dev-overlay/vite-plugin-dev-overlay.js
@@ -12,7 +12,7 @@ function astroDevOverlay({ settings }) {
       if (id === resolvedVirtualModuleId) {
         return `
 					export const loadDevOverlayPlugins = async () => {
-						return [${settings.devToolbarApps.map((plugin) => `(await import('${plugin}')).default`).join(",")}];
+						return [${settings.devToolbarApps.map((plugin) => `(await import(${JSON.stringify(plugin)})).default`).join(",")}];
 					};
 				`;
       }

There's another problem on @spotlightjs/astro's side, fixed similarly via:

--- a/src/snippets.ts
+++ b/src/snippets.ts
@@ -9,7 +9,7 @@ export type ClientInitOptions = {
   injectImmediately?: boolean;
 } & SpotlightAstroIntegrationOptions;
 
-const buildClientImport = (importPath: string) => `import * as Spotlight from '${importPath}';`;
+const buildClientImport = (importPath: string) => `import * as Spotlight from ${JSON.stringify(importPath)};`;
 
 const buildClientInit = (options: ClientInitOptions) => {
   const integrationCalls = options.integrationNames

(They did another import on their own for some reason, but with the same problem)

Together these two patches make Dev Toolbar & Spotlight integration show correctly on my environment. Also the lighthouse integration mentioned in the original issue also works now.

@bluwy
Copy link
Member Author

bluwy commented Dec 15, 2023

Awesome, thank you for testing the fix! I'm away from my laptop until next week, but I'll open this PR for review for now so we could get it out, and we could add tests later.

Also feel free to submit a PR to @apotlightjs/Astro to fix that! I didn't notice they did something similar too.

@bluwy bluwy marked this pull request as ready for review December 15, 2023 14:06
@ematipico ematipico merged commit 1e98438 into main Dec 18, 2023
13 checks passed
@ematipico ematipico deleted the fix-dev-toolbar-integrations branch December 18, 2023 13:41
@astrobot-houston astrobot-houston mentioned this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev Toolbar Apps failed to import on Windows
3 participants