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

fix: npmDependencies returns dependencies from .nx/installation/node_modules if node_modules is a file #2152

Conversation

mkapal
Copy link
Contributor

@mkapal mkapal commented May 28, 2024

When writing unit tests for npmDependencies, I decided to replace await stat(x)).isDirectory() with directoryExists(x) from @nx-console/shared/file-system and added a readDirectory function to make it easier to mock those file system operations.

Since the directoryExists function already handles thrown exceptions inside, I could refactor the main code by getting rid of the nested try...catch blocks, but then I noticed a strange code path: Before, if node_modules was not a directory but an existing file, an empty array was returned regardless whether .nx/installation/node_modules directory existed. Now it goes on to check if .nx/installation/node_modules exists too. I think this is the correct behavior now, albeit quite an unlikely scenario.

You can check the new test cases to see if my understanding is correct.

…de_modules` if `node_modules` exists but is not a directory
Copy link

nx-cloud bot commented May 28, 2024

@MaxKless MaxKless self-requested a review June 5, 2024 14:52
Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the refactor and the tests @mkapal
Since we have tests now, I think let's add another case: There is some logic for handling nested dependencies that start with @. Can you add another test case for that code path?
Thank you!

@mkapal
Copy link
Contributor Author

mkapal commented Jun 5, 2024

Good idea, I've added that now:

it('should return scoped packages starting with @', async () => {

@MaxKless MaxKless self-requested a review June 5, 2024 16:21
Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

looks great, thanks so much!

@MaxKless MaxKless merged commit 7eb76f9 into nrwl:master Jun 6, 2024
4 checks passed
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.

None yet

2 participants