feat: move excluded_patterns into function_config #274
Conversation
node/validation/manifest/schema.ts
Outdated
| @@ -24,6 +24,22 @@ const routesSchema = { | |||
| additionalProperties: false, | |||
| } | |||
|
|
|||
| const functionConfigSchema = { | |||
| type: 'object', | |||
| required: ['excluded_patterns'], | |||
There was a problem hiding this comment.
🤷 I don't remember. Removed it in 0fa20a9.
| importMap, | ||
| layers = [], | ||
| }: GenerateManifestOptions) => { | ||
| const preCacheRoutes: Route[] = [] | ||
| const postCacheRoutes: Route[] = [] | ||
| const manifestFunctionConfig: Manifest['function_config'] = Object.fromEntries( |
There was a problem hiding this comment.
I think this means that there will be an entry in function_config for every function, even if the object is empty. Is that intended? Could we simply omit a function from the object if it doesn't have any configuration properties?
There was a problem hiding this comment.
Thought the same when I wrote this first. Then I replaced it with 7c49753, found that to be too much indirection, and went back. Committed it again, let me know what you think ^^
| @@ -44,13 +44,13 @@ export const getDeclarationsFromConfig = ( | |||
| const paths = Array.isArray(config.path) ? config.path : [config.path] | |||
|
|
|||
| paths.forEach((path) => { | |||
| declarations.push({ ...declaration, ...config, path }) | |||
| declarations.push({ ...declaration, cache: config.cache, path }) | |||
There was a problem hiding this comment.
Not sure I understand this bit. Why are we suddenly setting the cache property specifically rather than all of config? And does it mean that if we add more configuration properties to ISC, will we have to add them here in addition to config.ts?
There was a problem hiding this comment.
This is necessary because config now not only contains cache and path, but also excludedPatterns, which should not be part of the declaration. This means that we'll have to manually add all declaration-specific ISC properties here, yes.
Now that I think about it, should preCache / postCache be part of functions config as well? Are there cases where the same edge function sometimes should run before, and sometimes after cache? (this is out of scope for this PR)
There was a problem hiding this comment.
This is necessary because
confignow not only containscacheandpath, but alsoexcludedPatterns, which should not be part of thedeclaration.
I guess you could push to the declaration everything except excludedPatterns, which you know shouldn't be there. If we add more properties to ISC we wouldn't need to manually include them here?
Happy to defer to you though.
node/manifest.ts
Outdated
| importMap, | ||
| layers = [], | ||
| }: GenerateManifestOptions) => { | ||
| const preCacheRoutes: Route[] = [] | ||
| const postCacheRoutes: Route[] = [] | ||
| const manifestFunctionConfig: Manifest['function_config'] = {} | ||
|
|
||
| const getFunctionConfig = (name: string) => { |
There was a problem hiding this comment.
I find this a bit confusing. We have a nested function that mutates an object in the outer scope but its name says it's a getter.
If I understand this correctly, the goal of this function is to prime manifestFunctionConfig with blank entries that you can safely mutate? Perhaps it would be nicer to make this function explicitly about that, by both changing the name and removing the return value so that it's explicitly a setter.
Then further below you could access manifestFunctionConfig directly rather than the return value of getFunctionConfig?
There was a problem hiding this comment.
Tried this out, and having a function called primeFunctionConfig also looked weird and added indirection. I tried a different approach where the config is filled up at the start, and then reduced before saving it: 520a241 How do you like that?
node/manifest.ts
Outdated
|
|
||
| for (const [name, functionConfig] of Object.entries(newConfig)) { | ||
| if (functionConfig.excluded_patterns.length === 0) { | ||
| delete newConfig[name] |
There was a problem hiding this comment.
Personally not a big fan of using delete here. I would rather start with an empty object for newConfig and then add the individual properties if excluded_patterns.length !== 0.
…er#274) * feat: move excluded_patterns into function_config * feat: isc-defined excluded_patterns * feat: remove "excluded_patterns" from required fields * feat: only write functionConfig for functions that need it * refactor: sanitize function config before saving * fix: use right object to build config map * refactor: add instead of delete
Which problem is this pull request solving?
Accompanies https://github.com/netlify/stargate/pull/1343. We found that having
excluded_patternsinside ofroutesdoesn't cut it, so we're moving it into a newfunction_configfield.List other issues or pull requests related to this problem