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

feat: enable framework detection on js workspace root #5853

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/build-info/src/frameworks/svelte.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down
104 changes: 104 additions & 0 deletions packages/build-info/src/mutex.test.ts
Original file line number Diff line number Diff line change
@@ -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)
})
})
88 changes: 88 additions & 0 deletions packages/build-info/src/mutex.ts
Original file line number Diff line number Diff line change
@@ -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<ReleaseFunction>((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<T>(callback: () => Promise<T>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could the same be achieved with just p-limit using 1 as concurrency? It seems like p-limit is already indirect dependency of build-info:

npm list p-limit
[email protected] /Users/misiek/dev/netlify-build
└─┬ @netlify/[email protected] -> ./packages/build-info
  ├─┬ @bugsnag/[email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └─┬ [email protected]
  │       └─┬ [email protected]
  │         └── [email protected]
  ├─┬ @vitest/[email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └─┬ [email protected]
  │       └─┬ [email protected]
  │         └── [email protected]
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └── [email protected] deduped
  └─┬ [email protected]
    └─┬ @vitest/[email protected]
      └── [email protected]

(no strong preference here, just checking)

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
28 changes: 24 additions & 4 deletions packages/build-info/src/project.ts
Original file line number Diff line number Diff line change
@@ -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'

Expand All @@ -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,
Expand Down Expand Up @@ -295,8 +297,17 @@ 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.workspace.packages.push({ path: '', name: 'root' })
this.frameworks.set('', rootFrameworks)
}
// if we have a workspace parallelize in all workspaces
await Promise.all(
this.workspace.packages.map(async ({ path: pkg, forcedFramework }) => {
Expand All @@ -308,15 +319,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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you briefly explain why mutex is needed here (and only here)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also - shouldn't this exclusive run be wider? index of item to remove is figured before entering exclusive mode and if mutex is here at all, presumably multiple concurrent runs can execute this code, so index figured out before acquiring mutex might no longer be correct because since figured out i something else might have mutated things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's a good point!

yea because we run this highly in parallel so mutating the array is dangerous in a concurrent highly async loop!

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) {
Expand Down
27 changes: 27 additions & 0 deletions packages/build-info/src/settings/get-build-settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
11 changes: 10 additions & 1 deletion packages/build-info/src/settings/get-build-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
Loading