From 7898f0168b393f440ee56627cda806026988a40d Mon Sep 17 00:00:00 2001 From: pieh Date: Wed, 18 Sep 2024 13:06:20 +0200 Subject: [PATCH 1/4] test: add check to see if all the frameworks honors forced framework --- packages/build-info/src/get-framework.test.ts | 17 ++++++++++++++++- packages/build-info/src/get-framework.ts | 3 +++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/build-info/src/get-framework.test.ts b/packages/build-info/src/get-framework.test.ts index 8cfd35beca..38682ad65d 100644 --- a/packages/build-info/src/get-framework.test.ts +++ b/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) + }) + } +}) diff --git a/packages/build-info/src/get-framework.ts b/packages/build-info/src/get-framework.ts index 16daba0f84..a346b6cd6d 100644 --- a/packages/build-info/src/get-framework.ts +++ b/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 From 25dd93223ec5fdb69b54dd7195c270b3fe59a34e Mon Sep 17 00:00:00 2001 From: pieh Date: Thu, 19 Sep 2024 12:05:56 +0200 Subject: [PATCH 2/4] fix: actually merge detections and not just pick up the most accurate one --- .../src/frameworks/framework.test.ts | 20 ++++++++++ .../build-info/src/frameworks/framework.ts | 40 ++++++++++++++----- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/packages/build-info/src/frameworks/framework.test.ts b/packages/build-info/src/frameworks/framework.test.ts index 32f202646f..bf9f47f2ab 100644 --- a/packages/build-info/src/frameworks/framework.test.ts +++ b/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', () => { diff --git a/packages/build-info/src/frameworks/framework.ts b/packages/build-info/src/frameworks/framework.ts index 82567b31ed..cb4a9f7ce4 100644 --- a/packages/build-info/src/frameworks/framework.ts +++ b/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 @@ -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 { - 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 { - // 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 From ce5817a3d35d8dc67d4e89b65fedc6edf9d0e651 Mon Sep 17 00:00:00 2001 From: pieh Date: Thu, 19 Sep 2024 12:11:21 +0200 Subject: [PATCH 3/4] refactor: use single detection pass with remix and hydrogen --- packages/build-info/src/frameworks/hydrogen.ts | 14 ++++---------- packages/build-info/src/frameworks/remix.ts | 14 ++++---------- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/packages/build-info/src/frameworks/hydrogen.ts b/packages/build-info/src/frameworks/hydrogen.ts index 24e31a550b..bb6b0996e9 100644 --- a/packages/build-info/src/frameworks/hydrogen.ts +++ b/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 = { @@ -43,23 +44,16 @@ export class Hydrogen extends BaseFramework implements Framework { async detect(): Promise { await super.detect() - if (this.detected) { - const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES) - if (viteDetection) { - this.detected = viteDetection + if (this.detected?.configName) { + if (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 if (CLASSIC_COMPILER_CONFIG_FILES.includes(this.detected.configName)) { 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 } } } diff --git a/packages/build-info/src/frameworks/remix.ts b/packages/build-info/src/frameworks/remix.ts index 0cdcea9157..5c14923a22 100644 --- a/packages/build-info/src/frameworks/remix.ts +++ b/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 = { @@ -54,23 +55,16 @@ export class Remix extends BaseFramework implements Framework { async detect(): Promise { await super.detect() - if (this.detected) { - const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES) - if (viteDetection) { - this.detected = viteDetection + if (this.detected?.configName) { + if (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 if (CLASSIC_COMPILER_CONFIG_FILES.includes(this.detected.configName)) { 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 } } } From 2a59c124c199eac6e7515a526248e4ddd8512ddb Mon Sep 17 00:00:00 2001 From: pieh Date: Thu, 19 Sep 2024 12:12:56 +0200 Subject: [PATCH 4/4] fix: honor forced remix and hydrogen framework and allow detecting when config file is missing --- .../build-info/src/frameworks/hydrogen.ts | 6 +-- .../build-info/src/frameworks/remix.test.ts | 54 ++++++++----------- packages/build-info/src/frameworks/remix.ts | 6 +-- 3 files changed, 28 insertions(+), 38 deletions(-) diff --git a/packages/build-info/src/frameworks/hydrogen.ts b/packages/build-info/src/frameworks/hydrogen.ts index bb6b0996e9..96bcf377d9 100644 --- a/packages/build-info/src/frameworks/hydrogen.ts +++ b/packages/build-info/src/frameworks/hydrogen.ts @@ -44,12 +44,12 @@ export class Hydrogen extends BaseFramework implements Framework { async detect(): Promise { await super.detect() - if (this.detected?.configName) { - if (VITE_CONFIG_FILES.includes(this.detected.configName)) { + if (this.detected) { + if (this.detected.configName && VITE_CONFIG_FILES.includes(this.detected.configName)) { this.dev = VITE_DEV this.build = VITE_BUILD return this as DetectedFramework - } else if (CLASSIC_COMPILER_CONFIG_FILES.includes(this.detected.configName)) { + } else { this.dev = CLASSIC_COMPILER_DEV this.build = CLASSIC_COMPILER_BUILD return this as DetectedFramework diff --git a/packages/build-info/src/frameworks/remix.test.ts b/packages/build-info/src/frameworks/remix.test.ts index 4ee96c15bf..ef75d2d5af 100644 --- a/packages/build-info/src/frameworks/remix.test.ts +++ b/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') -}) diff --git a/packages/build-info/src/frameworks/remix.ts b/packages/build-info/src/frameworks/remix.ts index 5c14923a22..de4471c99d 100644 --- a/packages/build-info/src/frameworks/remix.ts +++ b/packages/build-info/src/frameworks/remix.ts @@ -55,12 +55,12 @@ export class Remix extends BaseFramework implements Framework { async detect(): Promise { await super.detect() - if (this.detected?.configName) { - if (VITE_CONFIG_FILES.includes(this.detected.configName)) { + if (this.detected) { + if (this.detected.configName && VITE_CONFIG_FILES.includes(this.detected.configName)) { this.dev = VITE_DEV this.build = VITE_BUILD return this as DetectedFramework - } else if (CLASSIC_COMPILER_CONFIG_FILES.includes(this.detected.configName)) { + } else { this.dev = CLASSIC_COMPILER_DEV this.build = CLASSIC_COMPILER_BUILD return this as DetectedFramework