Skip to content

fix: improve types and remove path-exists dependency #6552

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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix: improve types and remove path-exists dependency #6552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a0c01a
chore: remove ESLint suppressions for functions-utils
outslept Jul 12, 2025
dbca3d9
refactor: improve TypeScript types and replace path-exists
outslept Jul 12, 2025
c7181ca
refactor: improve test TypeScript types and remove path-exists
outslept Jul 12, 2025
8138bce
fix: remove path-exists dependency from functions-utils
outslept Jul 12, 2025
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
20 changes: 0 additions & 20 deletions eslint_temporary_suppressions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2228,26 +2228,6 @@ export default [
'@typescript-eslint/no-unsafe-return': 'off',
},
},
{
files: ['packages/functions-utils/src/main.ts'],
rules: {
'@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-unsafe-return': 'off',
'@typescript-eslint/no-unsafe-call': 'off',
'@typescript-eslint/no-unsafe-argument': 'off',
},
},
{
files: ['packages/functions-utils/tests/main.test.ts'],
rules: {
'@typescript-eslint/no-unsafe-argument': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/restrict-template-expressions': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of these appear to still be necessary to suppress issues that have not yet been resolved
Running npm run lint returns ✖ 23 problems (23 errors, 0 warnings)
It does look as though this line specifically (i.e. '@typescript-eslint/restrict-template-expressions': 'off',) can be removed.

},
},
{
files: ['packages/git-utils/src/commits.ts'],
rules: {
Expand Down
3 changes: 1 addition & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions packages/functions-utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@
"license": "MIT",
"dependencies": {
"@netlify/zip-it-and-ship-it": "13.2.0",
"cpy": "^11.0.0",
"path-exists": "^5.0.0"
"cpy": "^11.0.0"
},
"devDependencies": {
"@types/node": "^18.19.111",
Expand Down
44 changes: 29 additions & 15 deletions packages/functions-utils/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,35 @@
import { promises as fs } from 'fs'
import { basename, dirname, join } from 'path'
import { access, stat } from 'fs/promises'

import { listFunctions, listFunctionsFiles } from '@netlify/zip-it-and-ship-it'
import cpy from 'cpy'
import { pathExists } from 'path-exists'

interface FailOptions {
fail?: (message: string, options?: { error?: unknown }) => void
}

// Add a Netlify Function file to the `functions` directory, so it is processed
// by `@netlify/plugin-functions-core`
export const add = async function (src?: string, dist?: string, { fail = defaultFail } = {}): Promise<void> {
export const add = async function (
src?: string,
dist?: string,
{ fail = defaultFail }: FailOptions = {},
): Promise<void> {
if (src === undefined) {
return fail('No function source directory was specified')
fail('No function source directory was specified')
return
}

if (!(await pathExists(src))) {
return fail(`No function file or directory found at "${src}"`)
try {
await access(src)
} catch {
fail(`No function file or directory found at "${src}"`)
return
}

if (dist === undefined) {
return fail('No function directory was specified')
fail('No function directory was specified')
return
}

const srcBasename = basename(src)
Expand All @@ -26,7 +38,7 @@ export const add = async function (src?: string, dist?: string, { fail = default
}

const getSrcAndDest = async function (src: string, srcBasename: string, dist: string): Promise<[string, string]> {
const srcStat = await fs.stat(src)
const srcStat = await stat(src)

if (srcStat.isDirectory()) {
return [`${srcBasename}/**`, join(dist, srcBasename)]
Expand All @@ -35,9 +47,10 @@ const getSrcAndDest = async function (src: string, srcBasename: string, dist: st
return [srcBasename, dist]
}

export const list = async function (functionsSrc, { fail = defaultFail } = {} as any) {
if (functionsSrc === undefined || functionsSrc.length === 0) {
return fail('No function directory was specified')
export const list = async function (functionsSrc: string | string[], { fail = defaultFail }: FailOptions = {}) {
if (Array.isArray(functionsSrc) ? functionsSrc.length === 0 : !functionsSrc) {
fail('No function directory was specified')
return
}

try {
Expand All @@ -47,9 +60,10 @@ export const list = async function (functionsSrc, { fail = defaultFail } = {} as
}
}

export const listAll = async function (functionsSrc, { fail = defaultFail } = {} as any) {
if (functionsSrc === undefined || functionsSrc.length === 0) {
return fail('No function directory was specified')
export const listAll = async function (functionsSrc: string | string[], { fail = defaultFail }: FailOptions = {}) {
if (Array.isArray(functionsSrc) ? functionsSrc.length === 0 : !functionsSrc) {
fail('No function directory was specified')
return
}

try {
Expand All @@ -59,6 +73,6 @@ export const listAll = async function (functionsSrc, { fail = defaultFail } = {}
}
}

const defaultFail = function (message) {
const defaultFail = function (message: string): never {
throw new Error(message)
}
38 changes: 31 additions & 7 deletions packages/functions-utils/tests/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { readFile, rm } from 'fs/promises'
import { readFile, rm, access } from 'fs/promises'
import { normalize, resolve } from 'path'
import { fileURLToPath } from 'url'

import cpy from 'cpy'
import { pathExists } from 'path-exists'
import sortOn from 'sort-on'
import { expect, test, vi } from 'vitest'

Expand All @@ -13,6 +12,15 @@ import { getDist, createDist } from './helpers/main.js'

const FIXTURES_DIR = fileURLToPath(new URL('fixtures', import.meta.url))

const pathExists = async (path: string): Promise<boolean> => {
try {
await access(path)
return true
} catch {
return false
}
}

test('Should copy a source file to a dist directory', async () => {
const dist = await getDist()
try {
Expand Down Expand Up @@ -45,7 +53,8 @@ test('Should throw when source is undefined', async () => {
test('Should throw when source is empty array', async () => {
const dist = await getDist()
try {
await expect(() => add([] as any, dist)).rejects.toThrow()
// @ts-expect-error testing invalid input
await expect(() => add([], dist)).rejects.toThrow()
} finally {
await rm(dist, { force: true, recursive: true })
}
Expand Down Expand Up @@ -101,13 +110,26 @@ test('Should overwrite dist file if it already exists', async () => {
})

test('Should allow "fail" option to customize failures', async () => {
const fail = vi.fn() as any
const fail = vi.fn()
await add(undefined, undefined, { fail })
expect(fail).toHaveBeenCalledOnce()
expect(fail).toHaveBeenCalledWith('No function source directory was specified')
})

const normalizeFiles = function (fixtureDir, { name, mainFile, runtime, extension, srcDir, srcFile, schedule }) {
interface FileData {
name: string
mainFile: string
runtime: string
extension: string
srcDir?: string
srcFile?: string
schedule?: string
}

const normalizeFiles = function (
fixtureDir: string,
{ name, mainFile, runtime, extension, srcDir, srcFile, schedule }: FileData,
) {
const mainFileA = normalize(`${fixtureDir}/${mainFile}`)
const srcFileA = srcFile === undefined ? {} : { srcFile: normalize(`${fixtureDir}/${srcFile}`) }
const srcDirA = srcDir ? { srcDir: resolve(fixtureDir, srcDir) } : {}
Expand All @@ -117,7 +139,8 @@ const normalizeFiles = function (fixtureDir, { name, mainFile, runtime, extensio
test('Can list function main files with list()', async () => {
const fixtureDir = `${FIXTURES_DIR}/list`
const functions = await list(fixtureDir)
expect(sortOn(functions, ['mainFile', 'extension'])).toEqual(
expect(functions).toBeDefined()
expect(sortOn(functions!, ['mainFile', 'extension'])).toEqual(
[
Copy link
Contributor

@mrstork mrstork Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Using functions! here gives me the following lint error:

This assertion is unnecessary since the receiver accepts the original type of the expression.eslint[@typescript-eslint/no-unnecessary-type-assertion](https://typescript-eslint.io/rules/no-unnecessary-type-assertion)

(side note for me to look into - I'm not sure why this isn't raising an error in CI)

Would you be able to share some context on this change? I'm thinking we should be able to stick with the original assertions

Copy link
Contributor Author

@outslept outslept Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's sort of weird, but I cannot reproduce. Ran npx eslint packages/functions-utils/tests/main.test.ts from root - no type assertion errors (doesn't show any errors at all for me for some reason)

Verified config via npx eslint --print-config (eslint --inspect-config works as well) - rule is enabled and set to [2] (error)

Even tried reinstalling the editor and still zero success.

Output tab in VS Code with eslint.debug enabled in .vscode/settings.json also shows nothing except ESLint working without errors.

image

And this is the npm run lint run from root

image

I'll try to reproduce in neovim.

Copy link
Contributor Author

@outslept outslept Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Was able to get the errors in Github Codespaces.

This still has some "quirks":

In Codespaces, running npm install → npm run lint shows the errors (including the @typescript-eslint/no-unnecessary-type-assertion you mentioned). However, after running npm run build, subsequent npm run lint runs show zero errors.

I know about --cache and stuff, but even without --cache eslint shows zero errors after build.

Copy link
Contributor Author

@outslept outslept Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To keep the experiment clean, I removed .eslintcache from .gitignore

image

Output is clean after running the command. And we get the error without !:

image

Copy link
Contributor Author

@outslept outslept Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hope this is helpful and can help us find the cause! I don't think I can dig any further. Perhaps this is caused by project references in Typescript?

https://www.typescriptlang.org/docs/handbook/project-references.html

Copy link
Contributor

Choose a reason for hiding this comment

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

When you have some time, could you please remove your .eslintcache and see if you can still reproduce?

Copy link
Contributor Author

@outslept outslept Jul 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nope, can only reproduce the same inconsistent ESLint behavior: git clone → npm install → npm run lint:ci shows errors, after rm .eslintcache -> npm run build → npm run lint:ci no previous errors are shown.

Locally I've migrated to projectService and added additional tsconfig files since some files weren't being caught properly, which was causing slow processing.

I'll open a separate PR with this proposal over the weekend.

Also this should be solved by that PR:

(side note for me to look into - I'm not sure why this isn't raising an error in CI)

UPD: I'll convert this to draft until this behavior is resolved

{ name: 'four', mainFile: 'four.js/four.js.js', runtime: 'js', extension: '.js', srcDir: 'four.js' },
{
Expand All @@ -138,7 +161,8 @@ test('Can list function main files with list()', async () => {
test('Can list all function files with listAll()', async () => {
const fixtureDir = `${FIXTURES_DIR}/list`
const functions = await listAll(fixtureDir)
expect(sortOn(functions, ['mainFile', 'extension'])).toEqual(
expect(functions).toBeDefined()
expect(sortOn(functions!, ['mainFile', 'extension'])).toEqual(
[
{
name: 'four',
Expand Down
Loading