Always include package.json in hash

[mehulkar]

We want to include each workspace's package.json as an input when
the user has configured a smaller set of inputs, because the package.json
defines the actual task turbo will execute for that workspace (via scripts).
If that task definition changes, the cache is invalid.

We only apply this inclusion when inputs are defined, because if inputs
are not defined, all git-tracked files are included in the hash,
which will also include the package.json file.

[vercel]

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

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Sep 9, 2022 at 6:39PM (UTC)

[mehulkar]

I spent some time thinking about how to write an e2e test for this, but other than a full scripts/e2e example, I couldn't think of something that would be worth while. I could add one in taskhash_test.go, but it involves a lot of mock data that would be manipulated in order to assert the behavior change; which at that point, you're just testing the test setup itself.

One way to test this change is how I've ben doing it locally with https://github.com/mehulkar/simplest-turbo (although you'll have to update TURBO_SRC_DIR in the ./run script.

[nathanhammond]

This is mutating the InputPatterns slice which is explicitly user input. For the inputs into our hash I'd prefer not to mutate the user input (so that we maintain that) and instead create a new slice that has the appended data.

e.g.:
calculatedInputs := make([]string, len(p.InputPatterns) + 1)

And then to populate:

for index, glob := range p.InputPatterns {
  calculatedInputs[index] = glob
}
calculatedInputs = append(calculatedInputs, "package.json")

We can also use take this opportunity to make sure we don't duplicate user-provided input patterns. globby actually has a weird side effect where it will return multiples for matches of different patterns. We run it through a set to dedupe for outputs but I'm not actually sure we do it for input patterns so we should take this opportunity to check to make sure that we're properly deduping the glob matches for inputs.

Also worth noting that append has one possibly surprising API footgun:
https://go.dev/play/p/qdmfQa8mSyy

[mehulkar]

Good idea. check out the latest commit and let me know if I've hit any footguns

[nathanhammond]

Re testing: TestGetPackageDeps already exists; if you add a package.json to the generated files I would expect to see that included in the hash. Can you do that as a "minimum viable test" approach?

[mehulkar]

: TestGetPackageDeps already exists; if you add a package.json to the generated files I would expect to see that included in the hash. Can you do that as a "minimum viable test" approach?

That's already there I think, unless I'm not understanding you https://github.com/vercel/turborepo/pull/1832/files#diff-9cc16f6cf68f549b9b0d39b005f2757d5c9be1b92b366f436f9ccf0684a7f68dR265

[nathanhammond]

I believe you've got enough feedback to ship this thing. Do so once you're ready!

[nathanhammond]

We've explicitly not adopted generics yet because of golang/go#48525. That issue is wrapping up so we can expect to upgrade Go and allow generics soon.

I'm super-excited to adopt them; but unfortunately, not yet.

[mehulkar]

ahh too bad, I was excited to learn them. Backed out and will read that issue to understand it more, thanks for the pointer!

[nathanhammond]

We should avoid logging in utility functions generally.

[nathanhammond]

Did I miss seeing this last time because of single commit mode or inattentiveness? I'm sorry.

[nathanhammond]

copyAndAppend is a bit too clever for Go.

Couple of things:

• I'm sorry, I forgot about copy, I'm usually doing a transformation of the slice contents along the way or pulling them out of a map. This case is different which leaves copy available.
• Even if there weren't copy Go loves verbosity so I generally would choose a copy then append pattern.

a := []int{1, 2, 3}
b := make([]int, len(a))
copy(b, a)
b = append(b, 6)
b[1] = 5
fmt.Printf("a: %v b: %v", a, b)

[mehulkar]

inlined, but chose to always copy the input, rather than conditionally. hope that's ok