Always include package.json in hash #1832

mehulkar · 2022-09-02T00:46:16Z

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 · 2022-09-02T00:46:19Z

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)

While working on another task (#1832), I found several things hard to read because of short variable names. Although types and "Go to Definition" helps in these cases, long function blocks and large files make it dfficult to focus on the task at hand. For example, in some cases, the variable `ht` and `th` appear side by side in the same block! This commit replaces some of these names to improve general readability (and also to reduce the diff in future behavior-changing work).

mehulkar · 2022-09-08T01:04:06Z

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 · 2022-09-08T04:38:28Z

cli/internal/hashing/package_deps_hash.go

+		// the package.json change (i.e. the tasks that turbo executes), we want
+		// a cache miss, since any existing cache could be invalid.
+		// Note this package.json will be resolved relative to the pkgPath.
+		p.InputPatterns = append(p.InputPatterns, "package.json")


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

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

nathanhammond · 2022-09-08T04:51:50Z

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 · 2022-09-08T20:18:29Z

: 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 · 2022-09-09T09:22:58Z

cli/internal/hashing/package_deps_hash.go

+// appends an item into a copy of an array to prevent mutating the original.
+// uses generics to take an array of any type and append an item of the same type.
+func copyAndAppend[K comparable](input []K, item K) []K {
+	output := make([]K, len(input))
+
+	for index, item := range input {
+		fmt.Printf("adding %#v at index %#v\n", item, index)
+		output[index] = item
+	}
+
+	output = append(output, item)
+	return output
+}


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.

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 · 2022-09-09T09:23:33Z

cli/internal/hashing/package_deps_hash.go

+	output := make([]K, len(input))
+
+	for index, item := range input {
+		fmt.Printf("adding %#v at index %#v\n", item, index)


We should avoid logging in utility functions generally.

nathanhammond · 2022-09-09T09:26:21Z

cli/internal/hashing/package_deps_hash_test.go

+	packageJSONPath := myPkgDir.Join("package.json")
+	err = packageJSONPath.WriteFile([]byte("{}"), 0644)
+	assert.NilError(t, err, "WriteFile")


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

nathanhammond · 2022-09-09T09:39:06Z

cli/internal/hashing/package_deps_hash.go

+
+// appends an item into a copy of an array to prevent mutating the original.
+// uses generics to take an array of any type and append an item of the same type.
+func copyAndAppend[K comparable](input []K, item K) []K {


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)

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

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.

This PR documents #1832 now that we're releasing it. **Edit: Do not merge this until 1.4.7 is released** In doing so, I'm also changing the structure of the docs to separate env and files, which also needs to happen for #1936. One intermediate unfortunate side effect is that the docs, for some time until #1936 is merged, will document `globalDependencies` separately in two sections. This may be a little confusing during this time frame.

mehulkar mentioned this pull request Sep 2, 2022

Replace some 2 letter variables for readability #1845

Merged

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from e6df3eb to 3655809 Compare September 2, 2022 22:00

vercel bot deployed to Preview September 2, 2022 22:01 View deployment

mehulkar changed the base branch from main to mk/2-letter-vars September 2, 2022 23:32

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from 3655809 to 0dc80db Compare September 2, 2022 23:32

vercel bot deployed to Preview September 2, 2022 23:32 View deployment

Base automatically changed from mk/2-letter-vars to main September 6, 2022 00:00

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from 0dc80db to 8acc5ef Compare September 6, 2022 18:53

vercel bot deployed to Preview September 6, 2022 18:54 View deployment

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from 8acc5ef to fe1bef1 Compare September 7, 2022 00:23

vercel bot deployed to Preview September 7, 2022 00:24 View deployment

mehulkar marked this pull request as ready for review September 8, 2022 00:58

mehulkar requested a review from a team as a code owner September 8, 2022 00:58

mehulkar requested review from tknickman and jaredpalmer September 8, 2022 00:58

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from ef8f37a to 7d05ba4 Compare September 8, 2022 01:07

vercel bot deployed to Preview September 8, 2022 01:08 View deployment

nathanhammond suggested changes Sep 8, 2022

View reviewed changes

mehulkar requested a review from nathanhammond September 8, 2022 20:18

nathanhammond approved these changes Sep 9, 2022

View reviewed changes

vercel bot deployed to Preview September 9, 2022 17:59 View deployment

gsoltis approved these changes Sep 9, 2022

View reviewed changes

mehulkar force-pushed the mehulkar/turbo-285-usage-of-inputs-should-automatically branch from 1412eb7 to d6512cb Compare September 9, 2022 18:38

mehulkar added the pr: automerge Kodiak will merge these automatically after checks pass label Sep 9, 2022

vercel bot deployed to Preview September 9, 2022 18:39 View deployment

mehulkar merged commit aa019da into main Sep 9, 2022

mehulkar deleted the mehulkar/turbo-285-usage-of-inputs-should-automatically branch September 9, 2022 19:28

mehulkar mentioned this pull request Sep 13, 2022

Document inclusion of package.json in workspace task cache keys #1955

Merged

mehulkar mentioned this pull request Oct 25, 2023

[turborepo] Allow users to customize which field changes in package.json will not affect cache hits #6132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Always include package.json in hash #1832

Always include package.json in hash #1832

Uh oh!

mehulkar commented Sep 2, 2022 •

edited

Loading

Uh oh!

vercel bot commented Sep 2, 2022 •

edited

Loading

Uh oh!

mehulkar commented Sep 8, 2022

Uh oh!

nathanhammond Sep 8, 2022 •

edited

Loading

Uh oh!

mehulkar Sep 8, 2022

Uh oh!

nathanhammond commented Sep 8, 2022

Uh oh!

mehulkar commented Sep 8, 2022

Uh oh!

nathanhammond left a comment

Uh oh!

nathanhammond Sep 9, 2022

Uh oh!

mehulkar Sep 9, 2022

Uh oh!

nathanhammond Sep 9, 2022

Uh oh!

nathanhammond Sep 9, 2022

Uh oh!

nathanhammond Sep 9, 2022

Uh oh!

mehulkar Sep 9, 2022

Uh oh!

Uh oh!

Always include package.json in hash #1832

Always include package.json in hash #1832

Uh oh!

Conversation

mehulkar commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehulkar commented Sep 8, 2022

Uh oh!

nathanhammond Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehulkar Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

nathanhammond commented Sep 8, 2022

Uh oh!

mehulkar commented Sep 8, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

nathanhammond Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

mehulkar Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

nathanhammond Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

nathanhammond Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

nathanhammond Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

mehulkar Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mehulkar commented Sep 2, 2022 •

edited

Loading

vercel bot commented Sep 2, 2022 •

edited

Loading

nathanhammond Sep 8, 2022 •

edited

Loading