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

refactor: simplify lookupFile #12585

Merged
merged 2 commits into from
Mar 26, 2023
Merged

refactor: simplify lookupFile #12585

merged 2 commits into from
Mar 26, 2023

Conversation

patak-dev
Copy link
Member

Description

After #12577 and #12576, there are only a few places where we are using lookupFile. Its API got quite complex and I think it is hiding the caller intention in some places. This PR simplifies the function. We may remove it later on, once we remove the CJS SSR build, it only will have a single use case (that could be replaced by a findNearestPackageData)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 26, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Comment on lines +30 to +33
const filePath = path.join(envDir, file)
if (!tryStatSync(filePath)?.isFile()) return []

return Object.entries(parse(fs.readFileSync(filePath)))
Copy link
Member Author

Choose a reason for hiding this comment

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

We were passing rootDir: envDir here, so only level would be checked by lookupFile but the call makes it seems env dirs are being looked up in the fs. Replaced it with a single tryStatSync

Comment on lines 1203 to 1214
function findNearestLockfile(dir: string) {
while (dir) {
for (const fileName of lockfileNames) {
const fullPath = path.join(dir, fileName)
if (tryStatSync(fullPath)?.isFile()) return fullPath
}
const parentDir = path.dirname(dir)
if (parentDir === dir) return

dir = parentDir
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can have a specialized version of the lookup here, this was the reason the second argument to lookupFile was an array in all other places.

Copy link
Member

Choose a reason for hiding this comment

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

I think It'd still prefer the array version instead so we don't have two similar code 🤔 There shouldn't be a huge perf hit for the case where an array isn't needed too.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, the function isn't used much now anyways

) {
return lookupFile(parentDir, formats, options)
export function lookupFile(dir: string, fileName: string): string | undefined {
while (dir) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the recursion, this is the same scheme used by @bluwy in findNearestPackageData.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Nice!

@bluwy bluwy merged commit 4215e22 into main Mar 26, 2023
@bluwy bluwy deleted the refactor/simplify-lookup-file branch March 26, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants