fix: improve types and remove path-exists dependency #6552
Conversation
Remove temporary TypeScript ESLint rule suppressions for: - packages/functions-utils/src/main.ts - packages/functions-utils/tests/main.test.ts
Replace the external `path-exists` dependency with native Node.js fs/promises access() function to eliminate external dependency. Add FailOptions interface to eliminate all unsafe type assignments. Updated import statements to use specific named imports instead of namespace imports. Update error patterns to use return statements after fail() calls.
Replace path-exists dependency in tests with native fs/promises access(). Add TypeScript interface for the normalizeFiles function to ensure type safety for params. Replace unsafe type assertions with TypeScript error suppression comment. Add null checks and type guards for list/listAll function results.
outslept
changed the title
|
off topic: based on the logs, the nudge action expects and regarding the failing edge-bundler test - it should be unrelated to my changes. |
Thank you! I'll look into correcting this separate from your changes
Correct, we unfortunately have some flakiness in our test suite currently. I've cleared that red x up for you |
| '@typescript-eslint/no-unsafe-argument': 'off', | ||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| '@typescript-eslint/no-unsafe-assignment': 'off', | ||
| '@typescript-eslint/restrict-template-expressions': 'off', |
There was a problem hiding this comment.
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.
| @@ -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( | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
And this is the npm run lint run from root
I'll try to reproduce in neovim.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To keep the experiment clean, I removed .eslintcache from .gitignore
Output is clean after running the command. And we get the error without !:
There was a problem hiding this comment.
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
There was a problem hiding this comment.
When you have some time, could you please remove your .eslintcache and see if you can still reproduce?
There was a problem hiding this comment.
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
🎉 Thanks for submitting a pull request! 🎉
Summary
Refactor the functions-utils package to fix linting & types and remove the external path-exists dependency.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)