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 resolveOptions to use project root instead of disk root (#7271) #8969

Closed
wants to merge 2 commits into from

Conversation

Kasea
Copy link

@Kasea Kasea commented Apr 28, 2023

Relates to (and closes?) #7271 (and similar)
Closes #8846

Changes resolveOptions function to use path.parse().dir as opposed to path.parse().root see docs for more information.

With the old code, it would resolve to C:/, D:/, ... etc instead of the root we're interested in. This caused it to climb the tree as opposed to descend it, which causes numerous problems.

In my case it would break my pnpm monorepo setup when trying to run/build with parcel. It would select my monorepo root (the first case of a lock file from D:/). I noticed I could solve it by putting any yarn.lock or package-lock.json inside of my app folder and it would select the correct root and build and start properly. This is due to the array LOCK_FILE_NAMES having pnpm-lock.yaml last.

💻 Examples

├─apps
│ └─examples
│ ├─package-lock.json
├─pnpm-lock.yaml
└─pnpm-workspace.yaml

@Kasea
Copy link
Author

Kasea commented May 3, 2023

@mischnic think it would be possible to have this released for next minor release? I'm fairly sure it should be a "patch" release, but some people might've done weird hacks to get around this issue, so don't want to go and potentially break those.

@Kasea
Copy link
Author

Kasea commented May 5, 2023

Sratch my previous comment, I no longer have any interest in Parcel; it's broken beyond repair.

I will leave the PR up in case someone wants to try and salvage this project

@devongovett
Copy link
Member

devongovett commented May 12, 2023

Sorry to hear that, what other issues did you have?

For this PR, I'm not sure this is right because the entry root (i.e. common directory between the entries), may not be the project root. For example, if the entries are /path/to/project/website/pages/a.js and /path/to/project/website/pages/b.js, then the common directory is /path/to/project/src/website/pages, and path.parse('/path/to/project/src/website/pages').dir is '/path/to/project/src/website'. But the project root should be '/path/to/project'. That's why we search upward from where the common path is up until we find a lock file or SCM directory, which indicates the project root.

@Kasea
Copy link
Author

Kasea commented May 12, 2023

won't get into it here and going backwards makes more sense than going upwards (you won't detect random files in the tree). You're more likely to have a random package-lock going up the tree than going down as the project itself will be checked first, which also gives you the option to debug faster and actually change how things get handled without changing things outside of the project.

@devongovett devongovett closed this Oct 9, 2023
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.

Autoinstall gets confused by symlinks and things break badly
2 participants