From 3d707136d88f5a8a05c6dad3a598280b95a1d52b Mon Sep 17 00:00:00 2001 From: Lukas Holzer Date: Thu, 26 Sep 2024 11:00:41 +0200 Subject: [PATCH] feat: enable framework detection on js workspace root Fixes FRB-793 It is valid to have a monorepo (PNPM workspace) but still have a site on the root of it This enables the support of still having detection inside monorepo packages but additionally on the root as well --- package.json | 3 +- packages/build-info/src/frameworks/svelte.ts | 1 + packages/build-info/src/mutex.test.ts | 104 ++++++++++++++++++ packages/build-info/src/mutex.ts | 88 +++++++++++++++ packages/build-info/src/project.ts | 27 ++++- .../src/settings/get-build-settings.test.ts | 27 +++++ .../src/settings/get-build-settings.ts | 11 +- 7 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 packages/build-info/src/mutex.test.ts create mode 100644 packages/build-info/src/mutex.ts diff --git a/package.json b/package.json index d8a8edb26a..8bbf0079d7 100644 --- a/package.json +++ b/package.json @@ -67,5 +67,6 @@ "lint-staged": { "!(packages/*/tests/**/fixtures/**/*)*.+(j|t)s": "eslint --ignore-path .gitignore --cache --fix", "*": "prettier --write --ignore-unknown" - } + }, + "packageManager": "pnpm@9.5.0+sha512.140036830124618d624a2187b50d04289d5a087f326c9edfc0ccd733d76c4f52c3a313d4fc148794a2a9d81553016004e6742e8cf850670268a7387fc220c903" } diff --git a/packages/build-info/src/frameworks/svelte.ts b/packages/build-info/src/frameworks/svelte.ts index 49b7ef0021..7f75f86015 100644 --- a/packages/build-info/src/frameworks/svelte.ts +++ b/packages/build-info/src/frameworks/svelte.ts @@ -5,6 +5,7 @@ export class Svelte extends BaseFramework implements Framework { name = 'Svelte' npmDependencies = ['svelte'] excludedNpmDependencies = ['sapper', '@sveltejs/kit'] + configFiles = ['svelte.config.js'] category = Category.FrontendFramework staticAssetsDirectory = 'static' diff --git a/packages/build-info/src/mutex.test.ts b/packages/build-info/src/mutex.test.ts new file mode 100644 index 0000000000..b9cd56947f --- /dev/null +++ b/packages/build-info/src/mutex.test.ts @@ -0,0 +1,104 @@ +import { describe, beforeEach, test, expect } from 'vitest' + +import { Mutex } from './mutex.js' + +describe('Mutex', () => { + let mutex: Mutex + + beforeEach(() => { + mutex = new Mutex() + }) + + test('should acquire and release the lock for a single operation', async () => { + let isLocked = false + + await mutex.runExclusive(async () => { + isLocked = mutex['_isLocked'] + expect(isLocked).toBe(true) + }) + + isLocked = mutex['_isLocked'] + expect(isLocked).toBe(false) + }) + + test('should run operations sequentially', async () => { + const results: number[] = [] + + const task = (id: number) => { + return mutex.runExclusive(async () => { + results.push(id) + // Simulate async operation + await new Promise((resolve) => setTimeout(resolve, 10)) + }) + } + + await Promise.all([task(1), task(2), task(3)]) + + expect(results).toEqual([1, 2, 3]) + }) + + test('should prevent concurrent access to critical section', async () => { + let concurrentAccesses = 0 + let maxConcurrentAccesses = 0 + + const task = () => { + return mutex.runExclusive(async () => { + concurrentAccesses++ + if (concurrentAccesses > maxConcurrentAccesses) { + maxConcurrentAccesses = concurrentAccesses + } + // Simulate async operation + await new Promise((resolve) => setTimeout(resolve, 10)) + concurrentAccesses-- + }) + } + + await Promise.all([task(), task(), task()]) + + expect(maxConcurrentAccesses).toBe(1) + }) + + test('should release the lock even if an exception occurs', async () => { + let isLockedDuringError = false + + const failingTask = () => { + return mutex.runExclusive(async () => { + isLockedDuringError = mutex['_isLocked'] + throw new Error('Intentional Error') + }) + } + + await expect(failingTask()).rejects.toThrow('Intentional Error') + + const isLockedAfterError = mutex['_isLocked'] + expect(isLockedDuringError).toBe(true) + expect(isLockedAfterError).toBe(false) + }) + + test('should handle mixed successful and failing operations', async () => { + const results: string[] = [] + + const successfulTask = () => { + return mutex.runExclusive(async () => { + results.push('success') + await new Promise((resolve) => setTimeout(resolve, 10)) + }) + } + + const failingTask = () => { + return mutex.runExclusive(async () => { + results.push('fail') + await new Promise((resolve) => setTimeout(resolve, 10)) + throw new Error('Intentional Error') + }) + } + + // eslint-disable-next-line @typescript-eslint/no-empty-function + const tasks = [successfulTask(), failingTask().catch(() => {}), successfulTask()] + + await Promise.all(tasks) + + expect(results).toEqual(['success', 'fail', 'success']) + expect(mutex['_isLocked']).toBe(false) + }) +}) diff --git a/packages/build-info/src/mutex.ts b/packages/build-info/src/mutex.ts new file mode 100644 index 0000000000..7af4daf6f1 --- /dev/null +++ b/packages/build-info/src/mutex.ts @@ -0,0 +1,88 @@ +/** + * A lock for synchronizing async operations. + * Use this to protect a critical section + * from getting modified by multiple async operations + * at the same time. + */ +export class Mutex { + /** + * When multiple operations attempt to acquire the lock, + * this queue remembers the order of operations. + */ + private _queue: { + resolve: (release: ReleaseFunction) => void + }[] = [] + + private _isLocked = false + + /** + * Wait until the lock is acquired. + * @returns A function that releases the acquired lock. + */ + acquire() { + return new Promise((resolve) => { + this._queue.push({ resolve }) + this._dispatch() + }) + } + + /** + * Enqueue a function to be run serially. + * + * This ensures no other functions will start running + * until `callback` finishes running. + * @param callback Function to be run exclusively. + * @returns The return value of `callback`. + */ + async runExclusive(callback: () => Promise) { + const release = await this.acquire() + try { + return await callback() + } finally { + release() + } + } + + /** + * Check the availability of the resource + * and provide access to the next operation in the queue. + * + * _dispatch is called whenever availability changes, + * such as after lock acquire request or lock release. + */ + private _dispatch() { + if (this._isLocked) { + // The resource is still locked. + // Wait until next time. + return + } + const nextEntry = this._queue.shift() + if (!nextEntry) { + // There is nothing in the queue. + // Do nothing until next dispatch. + return + } + // The resource is available. + this._isLocked = true // Lock it. + // and give access to the next operation + // in the queue. + nextEntry.resolve(this._buildRelease()) + } + + /** + * Build a release function for each operation + * so that it can release the lock after + * the operation is complete. + */ + private _buildRelease(): ReleaseFunction { + return () => { + // Each release function make + // the resource available again + this._isLocked = false + // and call dispatch. + this._dispatch() + } + } +} + +type ReleaseFunction = () => void diff --git a/packages/build-info/src/project.ts b/packages/build-info/src/project.ts index a528c047b4..d6e5b22168 100644 --- a/packages/build-info/src/project.ts +++ b/packages/build-info/src/project.ts @@ -1,4 +1,5 @@ import type { Client, NotifiableError } from '@bugsnag/js' +import { isEqual } from 'lodash-es' import type { PackageJson } from 'read-pkg' import { SemVer, coerce, parse } from 'semver' @@ -11,6 +12,7 @@ import { frameworks } from './frameworks/index.js' import { getFramework } from './get-framework.js' import { Logger } from './logger.js' import { Severity, report } from './metrics.js' +import { Mutex } from './mutex.js' import { AVAILABLE_PACKAGE_MANAGERS, PkgManagerFields, @@ -295,8 +297,16 @@ export class Project { // This needs to be run first await this.detectBuildSystem() this.frameworks = new Map() + // A monorepo can have something on the repository root as well. + const rootFrameworks = await this.detectFrameworksInPath() + // Nx build system installs all dependencies on the root which would lead to double detection for root projects + const isNx = this.buildSystems.some(({ id }) => id === 'nx') + const rootFrameworkMutex = new Mutex() if (this.workspace) { + if (rootFrameworks.length > 0 && !isNx) { + this.frameworks.set('', rootFrameworks) + } // if we have a workspace parallelize in all workspaces await Promise.all( this.workspace.packages.map(async ({ path: pkg, forcedFramework }) => { @@ -308,15 +318,24 @@ export class Project { // noop framework not found } } else if (this.workspace) { - const result = await this.detectFrameworksInPath(this.fs.join(this.workspace.rootDir, pkg)) - this.frameworks.set(pkg, result) + const results = await this.detectFrameworksInPath(this.fs.join(this.workspace.rootDir, pkg)) + // if we detect a framework (with the same detection result in a path, instead of the root we need to remove it in the root) + for (const result of results) { + for (let i = 0, max = rootFrameworks.length; i < max; i++) { + if (isEqual(result.detected, rootFrameworks[i].detected)) { + rootFrameworkMutex.runExclusive(async () => { + this.frameworks.set(pkg, rootFrameworks.splice(i, 1)) + }) + } + } + } + this.frameworks.set(pkg, results) } }), ) } else { - this.frameworks.set('', await this.detectFrameworksInPath()) + this.frameworks.set('', rootFrameworks) } - await this.events.emit('detectFrameworks', this.frameworks) return [...this.frameworks.values()].flat() } catch (error) { diff --git a/packages/build-info/src/settings/get-build-settings.test.ts b/packages/build-info/src/settings/get-build-settings.test.ts index 8d81c05c9f..f66968e4c5 100644 --- a/packages/build-info/src/settings/get-build-settings.test.ts +++ b/packages/build-info/src/settings/get-build-settings.test.ts @@ -146,6 +146,33 @@ test('get dev command from npm scripts if defined inside a workspace setup', asy ]) }) +test('get settings for a project on the monorepo root', async ({ fs }) => { + const cwd = mockFileSystem({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + }), + 'packages/astro/package.json': JSON.stringify({ + name: 'astro', + dependencies: { astro: '*' }, + }), + 'svelte.config.js': '', + }) + fs.cwd = cwd + const project = new Project(fs, cwd) + // using '' to get the settings for the project root + const settings = await project.getBuildSettings('') + + expect(settings).toHaveLength(1) + expect(settings[0].name).toBe('Svelte') + + const project2 = new Project(fs, cwd) + // not using a base directory to get the settings for all projects + const settings2 = await project2.getBuildSettings() + expect(settings2).toHaveLength(2) + expect(settings2[0].name).toBe('Svelte') + expect(settings2[1].name).toBe('NPM + Astro packages/astro') +}) + describe.each([ { describeName: 'WebFS', diff --git a/packages/build-info/src/settings/get-build-settings.ts b/packages/build-info/src/settings/get-build-settings.ts index 9b92638cf1..59498acb1a 100644 --- a/packages/build-info/src/settings/get-build-settings.ts +++ b/packages/build-info/src/settings/get-build-settings.ts @@ -144,5 +144,14 @@ export async function getBuildSettings(project: Project, packagePath?: string): } const settings = await Promise.all(settingsPromises) - return settings.filter(Boolean) as Settings[] + return settings.filter((setting) => { + // filter out settings that are not matching the packagePath + // for example we have a monorepo (where on the repository root is a site) + // - in this case the package path would be an empty '' string. + // - so we need to filter out the settings that are not matching the packagePath + if (!setting || (packagePath !== undefined && setting.packagePath !== packagePath)) { + return false + } + return true + }) }