Add config key invalidation #9597
Conversation
| @@ -20,6 +20,10 @@ type ConfigOpts = {| | |||
| env?: Environment, | |||
| result?: ConfigResult, | |||
| invalidateOnFileChange?: Set<ProjectPath>, | |||
| invalidateOnPackageKeyChange?: Array<{| | |||
There was a problem hiding this comment.
Would it make sense to make the invalidation part less “package key” specific and more generic “config key”? I’m thinking for plugin authors it could apply to other config files as well? The current implementation of pacakgeKey in getConfigFrom assumes pacakge.json, but the invalidation at least at the moment supports generic JSON, and could support other formats.
There was a problem hiding this comment.
Agreed. Updated the naming to be config key, rather than package key 👍
| let packageKeyNodes = this.packageKeyNodes.get(_filePath); | ||
| if (packageKeyNodes && (type === 'delete' || type === 'update')) { | ||
| for (let nodeId of packageKeyNodes) { | ||
| let isInvalid = type === 'delete'; |
There was a problem hiding this comment.
/nit this indentation is off - should prettier be run against this change?
There was a problem hiding this comment.
Is it? Prettier is passing for me and I have it setup. Not sure what you're referring to?
There was a problem hiding this comment.
Weird, the let isInvalid is showing the same indentation as the for .. { above, I'd expect it to be indented another level. Anyway, not going to argue with Prettier!
EDIT: huh, indentation is fine when I view the whole file.. must just be something about how GH renders the diff.
| fromProjectPath(options.projectRoot, filePath), | ||
| ); | ||
|
|
||
| if (conf == null || conf.config[packageKey] == null) { |
There was a problem hiding this comment.
The packageKey being removed could be a valid scenario couldn't it? Like if I remove custom config for a transformer that I no longer need because the defaults are file.
There was a problem hiding this comment.
Ahh yeah you're right, good catch.
| @@ -3,4 +3,5 @@ | |||
| export type FeatureFlags = {| | |||
| // This feature flag mostly exists to test the feature flag system, and doesn't have any build/runtime effect | |||
| +exampleFeature: boolean, | |||
| +packageKeyInvalidation: boolean, | |||
There was a problem hiding this comment.
Originally I had a "description" for feature flags as part of a richer FF object export, but decided it was unnecessary - however, it would be good to document what the feature flag is for in a comment here at least.
| assert.equal(b.changedAssets.size, 0); | ||
| }); | ||
|
|
||
| it('should invalidate when package.json config keys change', async function () { |
There was a problem hiding this comment.
Related to comment earlier, should invalidate when config key is removed as well.
| source: await options.inputFS.readFile(pkg.filePath, 'utf8'), | ||
| filePath: pkg.filePath, | ||
| data: conf?.contents, | ||
| source: await options.inputFS.readFile(conf.filePath, 'utf8'), |
There was a problem hiding this comment.
I think we talked about this during pairing, and it's definitely different for the transformer that does this, but this will only read the package.json here once when the packager is initialised, not per package?
There was a problem hiding this comment.
Once per build per worker I believe. However we can definitely improve this overreading in a separate PR.
mattcompiles
changed the title
↪️ Pull Request
Currently, when using config stored inside of package.json keys (utilised by the default JS transformer, bundler and JS packager) you end up invalidation your results when anything in the root package.json changes. This causes massive over invalidation, especially in large monorepos that store a huge list of deps and unrelated config in the root.
This PR attempts to address this by adding a new type of node to the RequestGraph called a
ConfigKeyNode. This node acts similar to the existingFileNode, however it also stores the package key and a content hash of it's value. When a plugin uses theconfig.loadConfigFromAPI and it resolves to something using thepackageKeyoption this new type of invalidation will be used. When the config request executes, it will calculate a content hash for the package value and store it in the request graph. Then when a file change is detected for the relevant package.json, the file will be read and the content hash validated against the stored one before causing any invalidation to occur.Things to note:
✔️ PR Todo