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

Proposal for incremental path refactor #995

Merged
merged 15 commits into from
Apr 12, 2022
Merged

Conversation

gsoltis
Copy link
Contributor

@gsoltis gsoltis commented Apr 4, 2022

This PR is a proposal for how we can start to get a handle on the various kinds of paths that turbo uses.

My goal is the following:

  • Eventually ensure that an absolute, platform-dependent path is used any time we interact with the filesystem
    • This will allow us to guarantee that we don't break --cwd, which is likely currently broken
  • Eventually ensure that we can easily identify relative paths, both with respect to the monorepo root and to a particular package root
  • Be incrementally adoptable. There is a huge amount of surface area to this change, so it's important to be able to tackle it a piece at a time
  • Be auditable. The goal would be to eventually ban usage of path/filepath and a subset of os.* functions anywhere in the codebase outside path.go. It may even be possible to automate this in CI. Furthermore, entry points and exit points from our custom types (currently just AbsolutePath) are clearly marked, along with the intention of eventually migrating them, if appropriate (e.g. ToStringDuringMigration, UnsafeToAbsolutePath).
  • New PRs that interact with the filesystem should use fs.AbsolutePath, even if they need to use one of the migration functions to facilitate that. Hopefully this will force both the submitter and reviewer to focus on ensuring that the new code will work properly with --cwd.

I've done a few pieces of the migration to show what it would look like in the code base in a partially migrated state. path.go contains the AbsolutePath type, which ideally would be the sole way to interact with the filesystem. Please let me know what you think about the api and the migration overall.

@vercel
Copy link

vercel bot commented Apr 4, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/FbQMxozP7bpuengQW9GebAH3TEgv
✅ Preview: https://turbo-site-git-gsoltis-pathfirstpass.vercel.sh

@gsoltis gsoltis marked this pull request as ready for review April 4, 2022 03:54
@vercel
Copy link

vercel bot commented Apr 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Deployment Preview Updated
turbo-site Deployed View Apr 12, 2022 at 5:09PM (UTC)

// and is used to enfore correct path manipulation
type AbsolutePath string

func CheckedToAbsolutePath(s string) (AbsolutePath, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this SafeAbsolutePath?

@kodiakhq kodiakhq bot merged commit f28ccdd into main Apr 12, 2022
@kodiakhq kodiakhq bot deleted the gsoltis/path_first_pass branch April 12, 2022 17:35
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