Error when trying to cache outside of package - #182
Conversation
| // Return absolute paths, even if we ultimately return relative ones, so | ||
| // that we can do path string comparisons. | ||
| absolute: true, |
There was a problem hiding this comment.
Is the path string comparison only necessary if opts.throwIfOutsideCwd is true? I wonder if it would be much more complex to only override the absolute passed to fastGlob and revert to relative paths if needed, only if throwIfOutsideCwd is true.
There was a problem hiding this comment.
Done. I thought the code would be simpler to consistently have absolute paths, but it's not so bad.
| !path.startsWith(normalizedCwdWithTrailingSep) && | ||
| path !== normalizedCwd |
There was a problem hiding this comment.
There's probably something I'm not thinking of but why do we need these 2 separate checks instead of just !path.startsWith(normalizedCwd)?
There was a problem hiding this comment.
It's because if normalizedCwd is /foo, then we want to match /foo/child and /foo, but not /foox. Added a comment.
| throwIfOutsideCwd: true, | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof GlobOutsideCwdError) { |
There was a problem hiding this comment.
I think, for tools like this that are often misconfigured and where we want to give a clear error message in that case, throwing errors is probably a bad strategy for communicating known failure modes, because it's not particularly type safe or easy to remember / notice.
WDYT about starting to use Result types instead?
There was a problem hiding this comment.
Might be right, but I think I'd prefer to think about a refactoring like this some other time.
I decided to use exceptions for errors consistently because it seemed like it simplified error handling. Exceptions can always happen because of APIs we don't control, so they are in the mix regardless. So we use AggregateError to accumulate all of the errors we encounter as we execute the build graph (both known and unknown). And then at the top-level, we can just look at all the errors, and nicely format the known ones. I'm not sure that's so obviously bad.
|
I think after rebasing, the new errors will also need to add diagnostics |
| const absPath = opts.absolute | ||
| ? match.path | ||
| : pathlib.resolve(normalizedCwd, match.path); |
There was a problem hiding this comment.
Sorry, I was late in adding this comment, but with this refactor, does the comment 2. above still apply? Do the matches returned by fastGlob with absolute: true not preserve the remnant input pattern syntax? I think there were tests cases that cover that so perhaps that is the case.
There was a problem hiding this comment.
Yeah, absolute:true seems to remove the weird remnants. But it's possible there are cases where it doesn't, so I feel better normalizing always.
There was a problem hiding this comment.
Ooh I totally glanced over the match.path reassignment with the fs.normalize. Thanks for clarifying.
It's currently not possible to locally cache an output file that isn't inside of the package directory. We check for this case when we delete and throw, but not when we cache. So if you are caching but have cleaning disabled, we would silently weirdly save the output file to a parent directory, and then not restore it.
Now this is an error.
Note we could in theory do this during analysis, but I'm not 100% confident in my ability to correctly detect this case given all of the possible magic glob syntax, so for now it's safer to just do it at runtime. (see #64).
Also note we could in theory support caching files outside of the package root, but we'd have to do something like a tarball for the local cache, instead of simply copying into
.wireit/<script>/cache/<hash>. We should think carefully about whether we want to do that, though, so I'm not dealing with that for now.Fixes #181