Fix package substrings for getChangedPackages #1533
Conversation
|
@wvanrensselaer is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
| @@ -204,7 +204,8 @@ func getChangedPackages(changedFiles []string, packageInfos map[interface{}]*fs. | |||
| for _, changedFile := range changedFiles { | |||
| found := false | |||
| for pkgName, pkgInfo := range packageInfos { | |||
| if pkgName != util.RootPkgName && strings.HasPrefix(changedFile, pkgInfo.Dir) { | |||
| // Add trailing slash to avoid package substring conflicts: (e.g. react, react-dom) | |||
| if pkgName != util.RootPkgName && strings.HasPrefix(changedFile, pkgInfo.Dir+"/") { | |||
There was a problem hiding this comment.
Hey Will! I noticed this breaks one of the other tests where the changedFile is libs/libB (no trailing slash)... TBH I'm not sure how realistic that case is, but it might be better to check each full path segment in the dirname.
I didn't want to step on your PR, I made a commit on a fork here: 015cb34
Feel free to cherry pick (if you can do that from a fork), or just pull in the code. It includes a few changes to the unit tests that I think are needed too (the unit tests take packageInfos as part of ctx)
There was a problem hiding this comment.
Ah nice catch, yeah was wondering if that case could come about. Would need at least a package.json for turbo to consider a directory a package, but then the directory wouldn't be the changed file, the package.json would. But maybe I'm not thinking of an edge case.
Using OS path separator is a good call, wasn't sure if these paths were OS-scoped. But looks like Nathan put up a PR so I'll close this.
There was a problem hiding this comment.
@wvanrensselaer You're 100% correct that it's weird, but (without checking) I suspect that's in there to protect from a weird symlink-to-directory edge case we always have to watch out for.
Previously we didn't properly check for package prefixes being at actual file-addressing boundaries. This PR resolves the issue. Fixes #1528 Closes #1533 Co-authored-by: Will Van Rensselaer <35181014+wvanrensselaer@users.noreply.github.com> Co-authored-by: Joe Boyle <8823810+helloitsjoe@users.noreply.github.com>
See #1528
Changed files belonging to a package which was a substring of another package (e.g.
react,react-dom) were sometimes not included in the turbo changed packages.I haven't worked much in Golang, let me know if there's a more idiomatic way to handle this.