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

FR: Walk directories in leaf -> root order #4036

Open
evancharlton opened this issue Jan 13, 2025 · 2 comments
Open

FR: Walk directories in leaf -> root order #4036

evancharlton opened this issue Jan 13, 2025 · 2 comments

Comments

@evancharlton
Copy link

Problem

During the build process, esbuild walks the directory hierarchy, looking for package.json files along the way (in resolver.go). For reasons explained below, there are two problems with the current arrangement:

  1. The traversal happens in root -> leaf order. If it's being run in /Volumes/src/my-project/my-package, then it'll read:
    1. /package.json (if it exists)
    2. /Volumes/package.json (if it exists)
    3. /Volumes/src/package.json (if it exists)
    4. /Volumes/src/my-project/package.json (if it exists)
    5. /Volumes/src/my-project/my-package/package.json (if it exists)
  2. There's no way to stop the traversal. Other projects provide a way to designate a known "root", to prevent higher traversal.

This setup creates a problem for us. Our project exists within a monorepo, and we're building with BuildXL. We're working to enable the enforceSourceReadsUnderPackageRoots configuration option build flag, which prevents processes from reading outside of their declared dependency tree.

Proposal

The proposal is to invert the logic: read from the leaf down. Along the way, the process could be aborted with a similar mechanism to indicate "this is the root, please stop traversing" to ESLint.

This pattern has other precedence in configuration of other projects in the JS ecosystem.

Workarounds

Our current workaround is to add our top-level package.json to the allow-list of files which can be read outside of the declared dependencies. This is suboptimal, since it opens up the window for other processes to read the file when they shouldn't.

@evanw
Copy link
Owner

evanw commented Jan 13, 2025

Please provide a way to reproduce the problem you are encountering. There are many reasons why I ask for a way to reproduce the problem, including:

  1. If I can reproduce it myself then I can fully understand it without a lot of time-intensive back-and-forth on GitHub
  2. You have proposed a solution to your problem, but it may not be the only solution, and may not even be the most appropriate one for esbuild
  3. Having a reproduction means I have a way to verify if a hypothetical fix actually works or not
  4. Having a reproduction means I can potentially write an automated test to prevent it from breaking again in the future

For example, one thing I don't understand: there's some option called enforceSourceReadsUnderPackageRoots but it sounds like it... doesn't enforce that source reads are under package roots? Since you're implying that esbuild is doing it anyway even despite the enforcement that is supposed to prevent it from happening. So from my perspective, I'm not even sure whether this is a bug with BuildXL instead of esbuild or not.

Marking this issue as unactionable because no reproduction instructions were provided.

@evancharlton
Copy link
Author

Thanks for the prompt & thoughtful response! I'll put together a repro, but it might take a bit of time.

there's some option called enforceSourceReadsUnderPackageRoots but it sounds like it... doesn't enforce that source reads are under package roots?

Well, yes and no. The enforcement is that violations fail the build -- not that processes cannot read outside of their source roots. I had a non-public discussion about changing this behavior with the BuildXL team, but I'll convert that discussion into a public FR on the BuildXL side of things, too.

I'm not even sure whether this is a bug with BuildXL instead of esbuild or not.

From my perspective, neither product has an issue -- it's just that they have features which don't mesh well when used together. So, I have FRs out for both projects 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants