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(node-resolve): prevent infinite findPackageJson loop #1648

Closed
wants to merge 2 commits into from

Conversation

dasdeo
Copy link

@dasdeo dasdeo commented Dec 14, 2023

This is fixed by checking if the root directory (/) was already looped over.

Rollup Plugin Name: node-resolve

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

resolves #1647

Description

This is fixed by checking if the root directory (/) was already looped over.
@dasdeo dasdeo requested a review from tjenkinson as a code owner December 14, 2023 09:48
@shellscape shellscape changed the title In certain cirumstances findPackageJson will loop forever. fix(node-resolve): prevent infinite findPackageJson loop Jan 9, 2024
@shellscape
Copy link
Collaborator

Thanks for the PR! We'll need a test added for this before we can merge though. The test should probably recreate the situation as noted in the linked issue, and it should fail if the fix isn't present.

@dasdeo
Copy link
Author

dasdeo commented Jan 11, 2024

I would write a unit test if I knew how since this is using the file system.

Any resources on how you deal with that in this project?

@shellscape
Copy link
Collaborator

yeah most of our tests use file based fixtures for executing a build. I'd use those as a reference.

@dasdeo
Copy link
Author

dasdeo commented Jan 11, 2024

I just had this thought, I could be using my repository that I used to show the bug as a template for the fixture.

I might be adding the test case for this bug in the next couple of days.

Just to be sure, tests are located at https://github.com/rollup/plugins/tree/master/packages/node-resolve/test/fixtures right?

@shellscape
Copy link
Collaborator

Just to be sure, tests are located at https://github.com/rollup/plugins/tree/master/packages/node-resolve/test/fixtures right?

The fixtures are there. The tests are a directory level up

@shellscape
Copy link
Collaborator

@dasdeo will you have the chance to write a test for this change? I'd like to merge it, but we need to protect against future regressions.

@shellscape
Copy link
Collaborator

Closing as abandoned.

@shellscape shellscape closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop in combination with virtual module and missing package.json
2 participants