fix: honor forced framework for Remix and Hydrogen (#5837)

* test: add check to see if all the frameworks honors forced framework

* fix: actually merge detections and not just pick up the most accurate one

* refactor: use single detection pass with remix and hydrogen

* fix: honor forced remix and hydrogen framework and allow detecting when config file is missing

Author: pieh

packages/build-info/src/frameworks/framework.test.ts

@@ -205,6 +205,26 @@ describe('detection merging', () => {
       ]),
     ).toMatchObject({ accuracy: Accuracy.NPM, package: { name: 'a' } })
   })
+
+  test('keeps information about config and dependency from the most accurate detection that has it', () => {
+    expect(
+      mergeDetections([
+        undefined,
+        { accuracy: Accuracy.Config, config: '/absolute/path/config.js', configName: 'config.js' },
+        { accuracy: Accuracy.NPMHoisted, package: { name: 'b' } },
+        { accuracy: Accuracy.Forced },
+        { accuracy: Accuracy.NPM, package: { name: 'a' } },
+      ]),
+    ).toMatchObject({
+      // set by highest accuracy detection
+      accuracy: Accuracy.Forced,
+      // provided by the NPM detection - preferred over package provided by NPMHoisted
+      package: { name: 'a' },
+      // provided by the Config detection
+      config: '/absolute/path/config.js',
+      configName: 'config.js',
+    })
+  })
 })
 
 describe('framework sorting based on accuracy and type', () => {

packages/build-info/src/frameworks/framework.ts

@@ -33,8 +33,10 @@ export type Detection = {
   /** The NPM package that was able to detect it (high accuracy) */
   package?: { name: string; version?: SemVer }
   packageJSON?: PackageJson
-  /** The config file that is associated with the framework */
+  /** The absolute path to config file that is associated with the framework */
   config?: string
+  /** The name of config file that is associated with the framework */
+  configName?: string
 }
 
 export type FrameworkInfo = ReturnType<Framework['toJSON']>
@@ -140,11 +142,26 @@ export function sortFrameworksBasedOnAccuracy(a: DetectedFramework, b: DetectedF
   return sort
 }
 
-/** Merges a list of detection results based on accuracy to get the one with the highest accuracy */
+/** Merges a list of detection results based on accuracy to get the one with the highest accuracy that still contains information provided by all other detections */
 export function mergeDetections(detections: Array<Detection | undefined>): Detection | undefined {
-  return detections
-    .filter(Boolean)
-    .sort((a: Detection, b: Detection) => (a.accuracy > b.accuracy ? -1 : a.accuracy < b.accuracy ? 1 : 0))?.[0]
+  const definedDetections = detections
+    .filter(function isDetection(d): d is Detection {
+      return Boolean(d)
+    })
+    .sort((a: Detection, b: Detection) => (a.accuracy > b.accuracy ? -1 : a.accuracy < b.accuracy ? 1 : 0))
+
+  if (definedDetections.length === 0) {
+    return
+  }
+
+  return definedDetections.slice(1).reduce((merged, detection) => {
+    merged.config = merged.config ?? detection.config
+    merged.configName = merged.configName ?? detection.configName
+    merged.package = merged.package ?? detection.package
+    merged.packageJSON = merged.packageJSON ?? detection.packageJSON
+
+    return merged
+  }, definedDetections[0])
 }
 
 export abstract class BaseFramework implements Framework {
@@ -277,6 +294,7 @@ export abstract class BaseFramework implements Framework {
           // otherwise the npm dependency should have already triggered the detection
           accuracy: this.npmDependencies.length === 0 ? Accuracy.ConfigOnly : Accuracy.Config,
           config,
+          configName: this.project.fs.basename(config),
         }
       }
     }
@@ -289,14 +307,14 @@ export abstract class BaseFramework implements Framework {
    * - if `configFiles` is set, one of the files must exist
    */
   async detect(): Promise<DetectedFramework | undefined> {
-    // we can force frameworks as well
-    if (this.detected?.accuracy === Accuracy.Forced) {
-      return this as DetectedFramework
-    }
-
     const npm = await this.detectNpmDependency()
     const config = await this.detectConfigFile(this.configFiles ?? [])
-    this.detected = mergeDetections([npm, config])
+    this.detected = mergeDetections([
+      // we can force frameworks as well
+      this.detected?.accuracy === Accuracy.Forced ? this.detected : undefined,
+      npm,
+      config,
+    ])
 
     if (this.detected) {
       return this as DetectedFramework

packages/build-info/src/frameworks/hydrogen.ts

@@ -32,6 +32,7 @@ export class Hydrogen extends BaseFramework implements Framework {
   readonly id = 'hydrogen'
   name = 'Hydrogen'
   npmDependencies = ['@shopify/hydrogen']
+  configFiles = [...VITE_CONFIG_FILES, ...CLASSIC_COMPILER_CONFIG_FILES]
   category = Category.SSG
 
   logo = {
@@ -44,22 +45,15 @@ export class Hydrogen extends BaseFramework implements Framework {
     await super.detect()
 
     if (this.detected) {
-      const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES)
-      if (viteDetection) {
-        this.detected = viteDetection
+      if (this.detected.configName && VITE_CONFIG_FILES.includes(this.detected.configName)) {
         this.dev = VITE_DEV
         this.build = VITE_BUILD
         return this as DetectedFramework
-      }
-      const classicCompilerDetection = await this.detectConfigFile(CLASSIC_COMPILER_CONFIG_FILES)
-      if (classicCompilerDetection) {
-        this.detected = classicCompilerDetection
+      } else {
         this.dev = CLASSIC_COMPILER_DEV
         this.build = CLASSIC_COMPILER_BUILD
         return this as DetectedFramework
       }
-      // If neither config file exists, it can't be a valid Hydrogen site for Netlify anyway.
-      return
     }
   }
 }

packages/build-info/src/frameworks/remix.test.ts

@@ -234,6 +234,28 @@ beforeEach((ctx) => {
       }),
     },
   ] as const,
+  [
+    'with no config file',
+    {
+      'package.json': JSON.stringify({
+        scripts: {
+          build: 'remix build',
+          dev: 'remix dev',
+          start: 'netlify serve',
+          typecheck: 'tsc',
+        },
+        dependencies: {
+          '@remix-run/netlify-edge': '1.7.4',
+          remix: '1.7.4',
+          react: '18.2.0',
+          'react-dom': '18.2.0',
+        },
+        devDependencies: {
+          '@netlify/functions': '^1.0.0',
+        },
+      }),
+    },
+  ] as const,
 ].forEach(([description, files]) =>
   test(`detects a Remix Classic Compiler site ${description}`, async ({ fs }) => {
     const cwd = mockFileSystem(files)
@@ -249,35 +271,3 @@ beforeEach((ctx) => {
     expect(detected?.[0]?.dev?.port).toBeUndefined()
   }),
 )
-
-test('does not detect an invalid Remix site with no config file', async ({ fs }) => {
-  const cwd = mockFileSystem({
-    'package.json': JSON.stringify({
-      scripts: {
-        build: 'remix vite:build',
-        dev: 'remix vite:dev',
-        start: 'remix-serve ./build/server/index.js',
-      },
-      dependencies: {
-        '@netlify/remix-runtime': '^2.3.0',
-        '@remix-run/node': '^2.9.2',
-        '@remix-run/react': '^2.9.2',
-        '@remix-run/serve': '^2.9.2',
-        react: '^18.2.0',
-        'react-dom': '^18.2.0',
-      },
-      devDependencies: {
-        '@netlify/remix-edge-adapter': '^3.3.0',
-        '@remix-run/dev': '^2.9.2',
-        '@types/react': '^18.2.20',
-        '@types/react-dom': '^18.2.7',
-        vite: '^5.0.0',
-        'vite-tsconfig-paths': '^4.2.1',
-      },
-    }),
-  })
-  const detected = await new Project(fs, cwd).detectFrameworks()
-
-  const detectedFrameworks = (detected ?? []).map((framework) => framework.id)
-  expect(detectedFrameworks).not.toContain('remix')
-})

packages/build-info/src/frameworks/remix.ts

@@ -43,6 +43,7 @@ export class Remix extends BaseFramework implements Framework {
     '@remix-run/netlify-edge',
   ]
   excludedNpmDependencies = ['@shopify/hydrogen']
+  configFiles = [...VITE_CONFIG_FILES, ...CLASSIC_COMPILER_CONFIG_FILES]
   category = Category.SSG
 
   logo = {
@@ -55,22 +56,15 @@ export class Remix extends BaseFramework implements Framework {
     await super.detect()
 
     if (this.detected) {
-      const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES)
-      if (viteDetection) {
-        this.detected = viteDetection
+      if (this.detected.configName && VITE_CONFIG_FILES.includes(this.detected.configName)) {
         this.dev = VITE_DEV
         this.build = VITE_BUILD
         return this as DetectedFramework
-      }
-      const classicCompilerDetection = await this.detectConfigFile(CLASSIC_COMPILER_CONFIG_FILES)
-      if (classicCompilerDetection) {
-        this.detected = classicCompilerDetection
+      } else {
         this.dev = CLASSIC_COMPILER_DEV
         this.build = CLASSIC_COMPILER_BUILD
         return this as DetectedFramework
       }
-      // If neither config file exists, it can't be a valid Remix site for Netlify anyway.
-      return
     }
   }
 }

packages/build-info/src/get-framework.test.ts

@@ -1,7 +1,8 @@
-import { test, expect, beforeEach } from 'vitest'
+import { describe, test, expect, beforeEach } from 'vitest'
 
 import { mockFileSystem } from '../tests/mock-file-system.js'
 
+import { frameworks } from './frameworks/index.js'
 import { getFramework } from './get-framework.js'
 import { NodeFS } from './node/file-system.js'
 import { Project } from './project.js'
@@ -52,3 +53,17 @@ test('should throw if a unknown framework was requested', async ({ fs }) => {
   }
   expect.assertions(1)
 })
+
+describe('Framework detection honors forced framework', () => {
+  for (const Framework of frameworks) {
+    test(Framework.name, async ({ fs }) => {
+      const cwd = mockFileSystem({})
+      const project = new Project(fs, cwd)
+      const framework = new Framework(project)
+
+      const detectedFramework = await getFramework(framework.id, project)
+
+      expect(detectedFramework.id).toBe(framework.id)
+    })
+  }
+})

packages/build-info/src/get-framework.ts

@@ -14,6 +14,9 @@ export async function getFramework(frameworkId: FrameworkName, project: Project)
     if (detected) {
       return detected
     }
+
+    // this indicate that framework's custom "detect" method doesn't honor the forced framework
+    throw new Error(`Forced framework "${frameworkId}" was not detected.`)
   }
 
   const frameworkIds = frameworkList