Merge pull request #7784 from QwikDev/v2-throw-duplicate

chore: warn when importing core twice

Author: wmertens

.changeset/lemon-dingos-dance.md

@@ -0,0 +1,5 @@
+---
+'@builder.io/qwik': minor
+---
+
+BREAKING: (slightly) Qwik will no longer scan all modules at build start to detect Qwik modules. Instead, a runtime check is done to prevent duplicate core imports. If you get a runtime error, you need to fix your build settings so they don't externalize qwik-related libraries.

packages/docs/src/repl/worker/repl-dependencies.ts

@@ -77,6 +77,8 @@ const _loadDependencies = async (replOptions: ReplInputOptions) => {
   }
 
   if (!isSameQwikVersion(self.qwikCore?.version)) {
+    // Special case: we allow importing qwik again in the same process.
+    delete (globalThis as any).__qwik;
     await exec(QWIK_PKG_NAME, '/core.cjs');
     if (self.qwikCore) {
       console.debug(`Loaded ${QWIK_PKG_NAME}: ${self.qwikCore.version}`);

packages/docs/vite.config.ts

@@ -137,6 +137,9 @@ export default defineConfig(async () => {
         'algoliasearch',
         '@algolia/autocomplete-core/dist/esm/reshape',
         'algoliasearch/dist/algoliasearch-lite.esm.browser',
+        'qwik-image',
+        '@modular-forms/qwik',
+        '@qwik-ui/headless',
       ],
     },
 

packages/qwik-router/src/ssg/worker-thread.ts

@@ -14,6 +14,9 @@ import type {
 } from './types';
 
 export async function workerThread(sys: System) {
+  // Special case: we allow importing qwik again in the same process, it's ok because we just needed the serializer
+  // TODO: remove this once we have vite environment API and no longer need the serializer separately
+  delete (globalThis as any).__qwik;
   const ssgOpts = sys.getOptions();
   const pendingPromises = new Set<Promise<any>>();
 
@@ -42,6 +45,9 @@ export async function workerThread(sys: System) {
 }
 
 export async function createSingleThreadWorker(sys: System) {
+  // Special case: we allow importing qwik again in the same process, it's ok because we just needed the serializer
+  // TODO: remove this once we have vite environment API and no longer need the serializer separately
+  delete (globalThis as any).__qwik;
   const ssgOpts = sys.getOptions();
   const pendingPromises = new Set<Promise<any>>();
 

packages/qwik/src/core/index.ts

@@ -1,3 +1,16 @@
+//////////////////////////////////////////////////////////////////////////////////////////
+// Protect against duplicate imports
+//////////////////////////////////////////////////////////////////////////////////////////
+import { version } from './version';
+if ((globalThis as any).__qwik) {
+  console.error(
+    `==============================================\n` +
+      `Qwik version ${(globalThis as any).__qwik} already imported while importing ${version}. Verify external vs bundled imports etc. This can lead to issues due to duplicated shared structures.\n` +
+      `==============================================\n`
+  );
+}
+(globalThis as any).__qwik = version;
+
 //////////////////////////////////////////////////////////////////////////////////////////
 // Developer Core API
 //////////////////////////////////////////////////////////////////////////////////////////

packages/qwik/src/optimizer/src/plugins/vite.ts

@@ -5,7 +5,6 @@ import type {
   GlobalInjections,
   Optimizer,
   OptimizerOptions,
-  OptimizerSystem,
   QwikManifest,
   TransformModule,
 } from '../types';
@@ -25,7 +24,6 @@ import {
   type NormalizedQwikPluginOptions,
   type QwikBuildMode,
   type QwikBuildTarget,
-  type QwikPackages,
   type QwikPluginOptions,
 } from './plugin';
 import { createRollupError, normalizeRollupOutputOptions } from './rollup';
@@ -96,7 +94,6 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
     async config(viteConfig, viteEnv) {
       await qwikPlugin.init();
 
-      const sys = qwikPlugin.getSys();
       const path = qwikPlugin.getPath();
 
       let target: QwikBuildTarget;
@@ -149,8 +146,6 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
       if (input && typeof input === 'string') {
         input = [input];
       }
-      const shouldFindVendors =
-        !qwikViteOpts.disableVendorScan && (target !== 'lib' || viteCommand === 'serve');
       viteAssetsDir = viteConfig.build?.assetsDir;
       const useAssetsDir = target === 'client' && !!viteAssetsDir && viteAssetsDir !== '_astro';
       const pluginOpts: QwikPluginOptions = {
@@ -208,8 +203,6 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
         clientDevInput = qwikPlugin.normalizePath(clientDevInput);
       }
 
-      const vendorRoots = shouldFindVendors ? await findQwikRoots(sys, sys.cwd()) : [];
-      const vendorIds = vendorRoots.map((v) => v.id);
       const isDevelopment = buildMode === 'development';
       const qDevKey = 'globalThis.qDev';
       const qTestKey = 'globalThis.qTest';
@@ -221,17 +214,11 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
 
       const updatedViteConfig: UserConfig = {
         ssr: {
-          noExternal: [
-            QWIK_CORE_ID,
-            QWIK_CORE_INTERNAL_ID,
-            QWIK_CORE_SERVER,
-            QWIK_BUILD_ID,
-            ...vendorIds,
-          ],
+          noExternal: [QWIK_CORE_ID, QWIK_CORE_INTERNAL_ID, QWIK_CORE_SERVER, QWIK_BUILD_ID],
         },
         envPrefix: ['VITE_', 'PUBLIC_'],
         resolve: {
-          dedupe: [...DEDUPE, ...vendorIds],
+          dedupe: [...DEDUPE],
           conditions: buildMode === 'production' && target === 'client' ? ['min'] : [],
           alias: {
             '@builder.io/qwik': '@qwik.dev/core',
@@ -264,8 +251,6 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
             QWIK_JSX_DEV_RUNTIME_ID,
             QWIK_BUILD_ID,
             QWIK_CLIENT_MANIFEST_ID,
-            // Sadly we can't specify **/*.qwik.*, so we need to specify each one
-            ...vendorIds,
             // v1 imports, they are removed during transform but vite doesn't know that
             '@builder.io/qwik',
             '@builder.io/qwik-city',
@@ -653,7 +638,87 @@ export function qwikVite(qwikViteOpts: QwikVitePluginOptions = {}): any {
     },
   } as const satisfies VitePlugin<QwikVitePluginApi>;
 
-  return [vitePluginPre, vitePluginPost];
+  return [vitePluginPre, vitePluginPost, checkExternals()];
+}
+
+/**
+ * This plugin checks for external dependencies that should be included in the server bundle,
+ * because they use Qwik. If they are not included, the optimizer won't process them, and there will
+ * be two instances of Qwik Core loaded.
+ */
+async function checkExternals() {
+  let fs: typeof import('fs').promises;
+  let path: typeof import('path');
+  try {
+    fs = await import('node:fs').then((m) => m.promises);
+    path = await import('node:path');
+  } catch {
+    // We can't do anything if we can't import fs and path
+    return;
+  }
+  const seen: Set<string> = new Set();
+  let rootDir: string;
+  const core2 = '@qwik-dev/core';
+  const core1 = '@builder.io/qwik';
+  async function isQwikDep(dep: string, dir: string) {
+    while (dir) {
+      const pkg = path.join(dir, 'node_modules', dep, 'package.json');
+      try {
+        await fs.access(pkg);
+        const data = await fs.readFile(pkg, {
+          encoding: 'utf-8',
+        });
+        // any mention of lowercase qwik in the package.json is enough
+        const json = JSON.parse(data);
+        if (
+          json.qwik ||
+          json.dependencies?.[core2] ||
+          json.peerDependencies?.[core2] ||
+          json.dependencies?.[core1] ||
+          json.peerDependencies?.[core1]
+        ) {
+          return true;
+        }
+        return false;
+      } catch {
+        //empty
+      }
+      const nextRoot = path.dirname(dir);
+      if (nextRoot === dir) {
+        break;
+      }
+      dir = nextRoot;
+    }
+    return false;
+  }
+
+  return {
+    name: 'checkQwikExternals',
+    enforce: 'pre',
+    configResolved: (config) => {
+      rootDir = config.root;
+    },
+    resolveId: {
+      order: 'pre',
+      async handler(source, importer, options) {
+        if (source.startsWith('node:') || seen.has(source)) {
+          return;
+        }
+        // technically we should check for each importer, but this is ok
+        seen.add(source);
+        const result = await this.resolve(source, importer, { ...options, skipSelf: true });
+        if (result?.external) {
+          // For Qwik files, check if they should be externalized
+          if (await isQwikDep(source, importer ? path.dirname(importer) : rootDir)) {
+            // TODO link to docs
+            throw new Error(
+              `\n==============\n${source} is being treated as an external dependency, but it should be included in the server bundle, because it uses Qwik.\nPlease add the package to "ssr.noExternal" in the Vite config. \n==============`
+            );
+          }
+        }
+      },
+    },
+  } as const satisfies VitePlugin<never>;
 }
 
 const ANSI_COLOR = {
@@ -715,90 +780,6 @@ export async function render(document, rootNode, opts) {
 }`;
 }
 
-async function findDepPkgJsonPath(sys: OptimizerSystem, dep: string, parent: string) {
-  const fs: typeof import('fs') = await sys.dynamicImport('node:fs');
-  let root = parent;
-  while (root) {
-    const pkg = sys.path.join(root, 'node_modules', dep, 'package.json');
-    try {
-      await fs.promises.access(pkg);
-      // use 'node:fs' version to match 'vite:resolve' and avoid realpath.native quirk
-      // https://github.com/sveltejs/vite-plugin-svelte/issues/525#issuecomment-1355551264
-      return fs.promises.realpath(pkg);
-    } catch {
-      //empty
-    }
-    const nextRoot = sys.path.dirname(root);
-    if (nextRoot === root) {
-      break;
-    }
-    root = nextRoot;
-  }
-  return undefined;
-}
-
-const findQwikRoots = async (
-  sys: OptimizerSystem,
-  packageJsonDir: string
-): Promise<QwikPackages[]> => {
-  const paths = new Map<string, string>();
-  if (sys.env === 'node' || sys.env === 'bun') {
-    const fs: typeof import('fs') = await sys.dynamicImport('node:fs');
-    let prevPackageJsonDir: string | undefined;
-    do {
-      try {
-        const data = await fs.promises.readFile(sys.path.join(packageJsonDir, 'package.json'), {
-          encoding: 'utf-8',
-        });
-
-        try {
-          const packageJson = JSON.parse(data);
-          const dependencies = packageJson['dependencies'];
-          const devDependencies = packageJson['devDependencies'];
-
-          const packages: string[] = [];
-          if (typeof dependencies === 'object') {
-            packages.push(...Object.keys(dependencies));
-          }
-          if (typeof devDependencies === 'object') {
-            packages.push(...Object.keys(devDependencies));
-          }
-
-          const basedir = sys.cwd();
-          await Promise.all(
-            packages.map(async (id) => {
-              const pkgJsonPath = await findDepPkgJsonPath(sys, id, basedir);
-              if (pkgJsonPath) {
-                const pkgJsonContent = await fs.promises.readFile(pkgJsonPath, 'utf-8');
-                const pkgJson = JSON.parse(pkgJsonContent);
-                const qwikPath = pkgJson['qwik'];
-                if (!qwikPath) {
-                  return;
-                }
-                // Support multiple paths
-                const allPaths = Array.isArray(qwikPath) ? qwikPath : [qwikPath];
-                for (const p of allPaths) {
-                  paths.set(
-                    await fs.promises.realpath(sys.path.resolve(sys.path.dirname(pkgJsonPath), p)),
-                    id
-                  );
-                }
-              }
-            })
-          );
-        } catch (e) {
-          console.error(e);
-        }
-      } catch {
-        // ignore errors if package.json not found
-      }
-      prevPackageJsonDir = packageJsonDir;
-      packageJsonDir = sys.path.dirname(packageJsonDir);
-    } while (packageJsonDir !== prevPackageJsonDir);
-  }
-  return Array.from(paths).map(([path, id]) => ({ path, id }));
-};
-
 export const isNotNullable = <T>(v: T): v is NonNullable<T> => {
   return v != null;
 };

packages/qwik/src/optimizer/src/plugins/vite.unit.ts

@@ -42,7 +42,6 @@ const noExternal = [
   '@qwik.dev/core/internal',
   '@qwik.dev/core/server',
   '@qwik.dev/core/build',
-  '@qwik.dev/router',
 ];
 
 const excludeDeps = [
@@ -53,7 +52,6 @@ const excludeDeps = [
   '@qwik.dev/core/jsx-dev-runtime',
   '@qwik.dev/core/build',
   '@qwik-client-manifest',
-  '@qwik.dev/router',
   '@builder.io/qwik',
   '@builder.io/qwik-city',
 ];

starters/dev-server.ts

@@ -43,10 +43,6 @@ const appNames = readdirSync(startersAppsDir).filter(
   (p) => statSync(join(startersAppsDir, p)).isDirectory() && p !== "base",
 );
 
-const rootDir = resolve(__dirname, "..");
-const packagesDir = resolve(rootDir, "packages");
-const qwikRouterMjs = join(packagesDir, "qwik-router", "lib", "index.qwik.mjs");
-
 /** Used when qwik-router server is enabled */
 const qwikRouterVirtualEntry = "@router-ssr-entry";
 const entrySsrFileName = "entry.ssr.tsx";
@@ -198,16 +194,7 @@ export {
       plugins: [
         ...plugins,
         optimizer.qwikVite({
-          /**
-           * normally qwik finds qwik-router via package.json but we don't want that
-           * because it causes it to try to lookup the special qwik router imports
-           * even when we're not actually importing qwik-router
-           */
-          disableVendorScan: true,
-          vendorRoots: enableRouterServer ? [qwikRouterMjs] : [],
-          entryStrategy: {
-            type: "segment",
-          },
+          entryStrategy: { type: "segment" },
           client: {
             manifestOutput(manifest) {
               clientManifest = manifest;
@@ -271,7 +258,8 @@ async function routerApp(
   appDir: string,
 ) {
   const ssrPath = join(appDir, "server", `${qwikRouterVirtualEntry}.js`);
-
+  // it's ok in the devserver to import core multiple times
+  (globalThis as any).__qwik = null;
   const mod = await import(file(ssrPath));
   const router: any = mod.router;
   router(req, res, () => {
@@ -289,6 +277,8 @@ async function ssrApp(
   manifest: QwikManifest,
 ) {
   const ssrPath = join(appDir, "server", "entry.ssr.js");
+  // it's ok in the devserver to import core multiple times
+  (globalThis as any).__qwik = null;
   const mod = await import(file(ssrPath));
   const render: Render = mod.default ?? mod.render;
 

starters/e2e/e2e.use-id.e2e.ts

@@ -1,4 +1,4 @@
-import { test, expect, Locator } from "@playwright/test";
+import { test, expect } from "@playwright/test";
 
 test.describe("use-id", () => {
   test.beforeEach(async ({ page }) => {