Various fixes

Adds the originating branch name to the PR branch name so we can differentiate by originating PRs.

FIX: don't await synchronous file append

FIX: Add all errors to result

FIX: Lockfile version retrieval fixes

Remove dates from file names

FIX: Don't push empty results to indirect deps

FIX: Write entire file

Clean data file before building data

Author: domoscargin

.github/workflows/update.yml

@@ -47,7 +47,11 @@ jobs:
       # github action which doesn't give us the option to output this data as
       # something like json which we can programmatically interogate.
       - name: Pipe our deps to a json file
-        run: github-dependents-info --repo alphagov/govuk-frontend --sort stars --json > ./data/raw-deps.json
+        run: github-dependents-info --repo alphagov/govuk-frontend --json > ./data/raw-deps.json
+
+      - name: Clean data files
+        run: |
+          rm -f ./data/filtered-data.json
 
       - name: Build filtered data
         run: npm run build-filtered-data
@@ -56,9 +60,9 @@ jobs:
         uses: peter-evans/create-pull-request@v6
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
-          branch: update-filtered-data
+          branch: update-filtered-data-${{ github.ref_name }}
           delete-branch: true
-          commit-message: "Get latest filtered dependents data and rejections"
-          title: "Get latest filtered dependents data and rejections"
+          commit-message: "Get latest filtered dependents data and rejections - ${{ github.ref_name }}"
+          title: "Get latest filtered dependents data and rejections - ${{ github.ref_name }}"
           body: "Generated automatically by github action"
           base: ${{ github.head_ref }}

build-filtered-data.mjs

@@ -1,4 +1,4 @@
-import { appendFileSync } from 'fs'
+import { writeFileSync } from 'fs'
 import { json2csv } from 'json-2-csv'
 import { RequestError } from 'octokit'
 
@@ -10,16 +10,9 @@ import { Result } from './helpers/result.mjs'
 
 import rawDeps from './data/raw-deps.json' with { type: 'json' }
 
-// Set up date for file naming
-const currentDate = new Date()
-const yyyymmdd = currentDate.toISOString().split('T')[0]
-const timestamp = currentDate.getTime()
-
 async function filterDeps () {
   const builtData = []
-  const batchSize = 500
   const processedIndexes = []
-  let batchCounter = 0
   console.log('Beginning dependency analysis...')
 
   for (const repo of rawDeps.all_public_dependent_repos) {
@@ -28,7 +21,6 @@ async function filterDeps () {
       const repoData = await analyseRepo(repo)
       if (repoData) {
         builtData.push(repoData)
-        batchCounter++
       }
       console.log(`${repo.name}: Analysis complete`)
 
@@ -49,23 +41,11 @@ async function filterDeps () {
         continue
       }
     }
-    if (batchCounter >= batchSize) {
-      await writeBatchToFiles(builtData)
-      builtData.length = 0
-      batchCounter = 0
-    }
   }
 
   if (builtData.length > 0) {
-    await writeBatchToFiles(builtData)
+    writeToFiles(builtData, processedIndexes)
   }
-
-  const unprocessedItems = rawDeps.all_public_dependent_repos.filter((_, index) => !processedIndexes.includes(index))
-
-  await appendFileSync(
-    `data/${yyyymmdd}-${timestamp}-unprocessedItems.json`,
-    JSON.stringify(unprocessedItems, null, 2)
-  )
   console.log("We're done!")
 }
 
@@ -109,22 +89,28 @@ export async function analyseRepo (repo) {
     }
   } catch (error) {
     repoData.handleError(error)
-    result.errorsThrown = repoData.errorsThrown
   }
+  result.errorsThrown = repoData.errorsThrown
 
   return result.getResult(repoData)
 }
 
-async function writeBatchToFiles (builtData) {
+function writeToFiles (builtData, processedIndexes) {
   // Write JSON file
-  await appendFileSync(
-    `data/${yyyymmdd}-${timestamp}-filtered-data.json`,
-    JSON.stringify(builtData, null, 2)
-  )
+  const jsonData = JSON.stringify(builtData, null, 2)
+  writeFileSync('data/filtered-data.json', jsonData)
+
   // Write CSV file
   const csv = json2csv(builtData)
-  await appendFileSync(`data/${yyyymmdd}-${timestamp}-filtered-data.csv`, csv)
+  writeFileSync('data/filtered-data.csv', csv)
   console.log('Data file updated with batch of entries')
+
+  // Write Unprocessed Items file
+  const unprocessedItems = rawDeps.all_public_dependent_repos.filter((_, index) => !processedIndexes.includes(index))
+  writeFileSync(
+    'data/unprocessedItems.json',
+    JSON.stringify(unprocessedItems, null, 2)
+  )
 }
 
 filterDeps()

helpers/repo-data.mjs

@@ -7,8 +7,6 @@ import { parse as parseYaml } from 'yaml'
 import yarnLock from '@yarnpkg/lockfile'
 import JSON5 from 'json5'
 
-export class UnsupportedLockfileError extends Error { }
-
 /**
  * The RepoData class is used to store and manipulate data about a repository, and serves as an abstraction
  * of the GitHub API.
@@ -31,7 +29,7 @@ export class RepoData {
     this.repoOwner = repoOwner
     this.repoName = repoName
     this.errorsThrown = []
-    this.lockfileUnsupported = false
+    this.rootLockfileVersion = null
   }
 
   /**
@@ -295,22 +293,30 @@ export class RepoData {
     const results = []
     // We want to try the root directory in all cases
     if (!packageObjects.some(pkg => pkg.path === 'package.json')) {
-      packageObjects.push({ path: '', content: {} })
+      packageObjects.push({ path: 'package.json', content: {} })
     }
     for (const packageObject of packageObjects) {
       try {
         const lockfileType = this.getLockfileType(packageObject.path, tree)
-        const lockfilePath = packageObject.path.replace('package.json', lockfileType)
-        const lockfile = await this.getRepoFileContent(packageObject.path.replace('package.json', lockfileType))
-        const lockfileObject = this.parseLockfile(lockfile, lockfileType)
-        results.push(await this.getIndirectDependencyFromLockfile(lockfileObject, lockfileType, lockfilePath))
+        if (lockfileType) {
+          const lockfilePath = packageObject.path.replace('package.json', lockfileType)
+          if (this.checkFileExists(lockfilePath, tree)) {
+            const lockfile = await this.getRepoFileContent(packageObject.path.replace('package.json', lockfileType))
+            const lockfileObject = this.parseLockfile(lockfile, lockfileType)
+            const deps = await this.getIndirectDependencyFromLockfile(lockfileObject, lockfileType, lockfilePath)
+            if (deps.length > 0) {
+              results.push(deps)
+            }
+          }
+        }
       } catch (error) {
         this.handleError(error)
       }
     }
     this.log(`${results.length} indirect dependencies found.`)
-
-    return results
+    if (results.length > 0) {
+      return results
+    }
   }
 
   /**
@@ -324,17 +330,28 @@ export class RepoData {
     this.log('disambiguating direct dependencies')
 
     const results = []
-    // We want to try the root directory in all cases
-    if (!dependencies.some(dep => dep.packagePath === 'package.json')) {
-      dependencies.push({ packagePath: '', specifiedVersion: '*' })
+    // We want to fall back to the root lockfile if we can't find a local lockfile
+    if (!this.rootLockfileVersion) {
+      const rootLockfileType = this.getLockfileType('package.json', tree)
+      if (rootLockfileType) {
+        const rootLockfile = await this.getRepoFileContent(rootLockfileType)
+        const rootLockfileObject = this.parseLockfile(rootLockfile, rootLockfileType)
+        this.rootLockfileVersion = await this.getLockfileVersion(rootLockfileObject, rootLockfileType, rootLockfileType, tree)
+      }
     }
+
     for (const dependency of dependencies) {
       try {
         if (/^[~^*]/.test(dependency.specifiedVersion)) {
           const lockfileType = this.getLockfileType(dependency.packagePath, tree)
-          const lockfile = await this.getRepoFileContent(dependency.packagePath.replace('package.json', lockfileType))
-          const lockfileObject = this.parseLockfile(lockfile, lockfileType)
-          dependency.actualVersion = await this.getLockfileVersion(lockfileObject, lockfileType, dependency.packagePath)
+          const lockfilePath = dependency.packagePath.replace('package.json', lockfileType)
+          if (this.checkFileExists(lockfilePath, tree)) {
+            const lockfile = await this.getRepoFileContent(lockfilePath)
+            const lockfileObject = this.parseLockfile(lockfile, lockfileType)
+            dependency.actualVersion = await this.getLockfileVersion(lockfileObject, lockfileType, dependency.packagePath, tree)
+          } else {
+            dependency.actualVersion = this.rootLockfileVersion ? this.rootLockfileVersion : dependency.specifiedVersion
+          }
         } else {
           dependency.actualVersion = dependency.specifiedVersion
         }
@@ -357,12 +374,13 @@ export class RepoData {
    * @param {string} path - The path to the package.
    * @returns {Promise<String>} The lockfile govuk-frontend semver string.
    */
-  async getLockfileVersion (lockfileObject, lockfileType, path) {
+  async getLockfileVersion (lockfileObject, lockfileType, path, tree) {
     let version
+
     if (lockfileType === 'package-lock.json') {
       version =
-          lockfileObject?.packages?.['node_modules/govuk-frontend']?.version ||
-          lockfileObject?.dependencies?.['govuk-frontend']?.version
+        lockfileObject?.packages?.['node_modules/govuk-frontend']?.version ||
+        lockfileObject?.dependencies?.['govuk-frontend']?.version
     } else if (lockfileType === 'yarn.lock') {
       const dependencyKey = Object.keys(lockfileObject).find(key => key.startsWith('govuk-frontend@'))
       version = lockfileObject[dependencyKey]?.version
@@ -456,14 +474,14 @@ export class RepoData {
    *
    * @returns {string} - the lockfile type
    */
-  getLockfileType (packagePath = '', tree) {
+  getLockfileType (packagePath = 'package.json', tree) {
     let lockfileType
-    if (this.checkFileExists('package-lock.json', tree) || this.checkFileExists(packagePath.replace('package.json', 'package-lock.json'), tree)) {
+    if (this.checkFileExists(packagePath.replace('package.json', 'package-lock.json'), tree)) {
       lockfileType = 'package-lock.json'
-    } else if (this.checkFileExists('yarn.lock', tree) || this.checkFileExists(packagePath.replace('package.json', 'yarn.lock'), tree)) {
+    } else if (this.checkFileExists(packagePath.replace('package.json', 'yarn.lock'), tree)) {
       lockfileType = 'yarn.lock'
     } else {
-      throw new UnsupportedLockfileError()
+      return null
     }
     return lockfileType
   }
@@ -476,9 +494,6 @@ export class RepoData {
    * @throws {Error} - If the error is not an expected type
    */
   handleError (error) {
-    if (error instanceof UnsupportedLockfileError) {
-      this.lockfileUnsupported = true
-    }
     this.log(`${error.message}. Added to result.`, 'error')
     this.errorsThrown.push(error.toString())
   }

helpers/repo-data.test.mjs

@@ -1,6 +1,6 @@
 import { describe, it, expect, vi } from 'vitest'
 import * as yarnLock from '@yarnpkg/lockfile'
-import { RepoData, UnsupportedLockfileError } from './repo-data.mjs'
+import { RepoData } from './repo-data.mjs'
 import {
   getFileContent,
   getTree,
@@ -283,15 +283,15 @@ describe('RepoData', () => {
       const repoData = new RepoData(repoOwner, repoName)
       const tree = [{ path: 'package-lock.json' }]
 
-      const lockfileType = repoData.getLockfileType('', tree)
+      const lockfileType = repoData.getLockfileType('package.json', tree)
       expect(lockfileType).toBe('package-lock.json')
     })
 
     it('should return yarn.lock if it exists', () => {
       const repoData = new RepoData(repoOwner, repoName)
       const tree = [{ path: 'yarn.lock' }]
 
-      const lockfileType = repoData.getLockfileType('', tree)
+      const lockfileType = repoData.getLockfileType('package.json', tree)
       expect(lockfileType).toBe('yarn.lock')
     })
 
@@ -311,11 +311,12 @@ describe('RepoData', () => {
       expect(lockfileType).toBe('yarn.lock')
     })
 
-    it('should throw Error if no supported lockfile exists', () => {
+    it('should return null if no supported lockfile exists', () => {
       const repoData = new RepoData(repoOwner, repoName)
       const tree = [{ path: 'other-file.lock' }]
 
-      expect(() => repoData.getLockfileType('', tree)).toThrow(UnsupportedLockfileError)
+      const lockfileType = repoData.getLockfileType('package.json', tree)
+      expect(lockfileType).toBe(null)
     })
   })
 
@@ -478,6 +479,7 @@ describe('RepoData', () => {
       repoData.getLockfileType = vi.fn().mockReturnValue('package-lock.json')
       repoData.getRepoFileContent = vi.fn().mockResolvedValue({ data: JSON.stringify(lockfileContent.data) })
       repoData.parseLockfile = vi.fn().mockReturnValue(lockfileContent.data)
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.getIndirectDependencies(packageObjects)
       expect(result).toEqual([[{
@@ -504,6 +506,7 @@ describe('RepoData', () => {
       }
       repoData.getLockfileType = vi.fn().mockReturnValue('yarn.lock')
       repoData.getRepoFileContent = vi.fn().mockResolvedValue(lockfileContent)
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.getIndirectDependencies(packageObjects)
       expect(result).toEqual([[{
@@ -522,9 +525,10 @@ describe('RepoData', () => {
       repoData.getLockfileType = vi.fn().mockReturnValue('package-lock.json')
       repoData.getRepoFileContent = vi.fn().mockRejectedValue(new Error('Failed to fetch lockfile content'))
       repoData.handleError = vi.fn()
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.getIndirectDependencies(packageObjects)
-      expect(result).toEqual([])
+      expect(result).toEqual(undefined)
       expect(repoData.handleError).toHaveBeenCalledWith(new Error('Failed to fetch lockfile content'))
     })
   })
@@ -546,6 +550,7 @@ describe('RepoData', () => {
       repoData.getLockfileType = vi.fn().mockReturnValue('package-lock.json')
       repoData.getRepoFileContent = vi.fn().mockResolvedValue(JSON.stringify(lockfileContent))
       repoData.parseLockfile = vi.fn().mockReturnValue(lockfileContent)
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.disambiguateDependencies(dependencies, tree)
       expect(result).toEqual([
@@ -569,6 +574,7 @@ describe('RepoData', () => {
       repoData.parseLockfile = vi.fn().mockReturnValue({
         'govuk-frontend@^3.11.0': { version: '3.11.0' }
       })
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.disambiguateDependencies(dependencies)
       expect(result).toEqual([
@@ -581,24 +587,11 @@ describe('RepoData', () => {
       const dependencies = [
         { packagePath: 'package.json', frontendVersion: '3.11.0' }
       ]
+      repoData.checkFileExists = vi.fn().mockReturnValue(true)
 
       const result = await repoData.disambiguateDependencies(dependencies)
       expect(result).toEqual(dependencies)
     })
-
-    it('should handle errors when fetching lockfile content', async () => {
-      const repoData = new RepoData(repoOwner, repoName)
-      const dependencies = [
-        { packagePath: 'package.json', specifiedVersion: '^3.11.0' }
-      ]
-      repoData.getLockfileType = vi.fn().mockReturnValue('package-lock.json')
-      repoData.getRepoFileContent = vi.fn().mockRejectedValue(new Error('Failed to fetch lockfile content'))
-      repoData.handleError = vi.fn()
-
-      const result = await repoData.disambiguateDependencies(dependencies)
-      expect(result).toEqual([])
-      expect(repoData.handleError).toHaveBeenCalledWith(new Error('Failed to fetch lockfile content'))
-    })
   })
 
   describe('getIndirectDependencyFromLockfile', () => {