feat: move non-route related ef configs to function_config in manifest

[khendrikse]

This is work for:

1. We used to define name and generator in the function declaration using the manifest file. But we're trying to steer more towards people using in-source-configs for anything that is NOT route related. We try to keep it backwards compatible for now
2. In the manifest, instead of passing name and generator to the routes/post_cache_routes arrays inside the routes object, we add it to the function_config.

List other issues or pull requests related to this problem

A piece of work for: https://github.com/netlify/pod-compute/issues/415

[eduardoboucas]

Left a couple of questions before doing a second pass.

[eduardoboucas]

I don't think "missing config fields" is very descriptive — what's missing and why? Perhaps we can add a comment explaining why we need this function?

[khendrikse]

I decided to switch it up a bit and make it, hopefully, more descriptive, and give functions a clear single task.

[eduardoboucas]

I don't love the fact that we're defining above what a config object is and then having another allow-list here, so adding a new property will mean adding it in two places. I can see people getting tripped up by that.

I don't have an obvious alternative to offer without getting into more complex schema validation, so I'm happy to defer to you.

[khendrikse]

I tried it this way because on this line we were checking some of the values.

What I could do, is allow for anything in the config to be passed to the function_config. This would mean that we get anything defined in it. Including cache: manual for example. Then we only need to filter out any config that is empty.

[eduardoboucas]

Why do we need to remove empty values?

[khendrikse]

I did this because the logic before this would allow for undefined values to be passed. For example. if we had on_error but functionConfig.excluded_patterns = [] we would otherwise still get that empty array added, or fields that were undefined. Like generator: undefined. Essentially I tried to optimise this line so I wouldn't have to add a bunch of other config values. And even if I did, if one of them was true, it would add undefined values for the others.

[danez]

So we are not allowing generator and name being set through the manifest? Why is that?

[danez]

FYI: I'm also working on the same code for splitting ISC up into internal vs user. https://github.com/netlify/edge-bundler/pull/347/files

[khendrikse]

So we are not allowing generator and name being set through the manifest? Why is that?

@danez the idea is that anything that is declared in the declarations for the manifest is mostly meant for routing information. ISC is for configuration on a function level.

[khendrikse]

FYI: I'm also working on the same code for splitting ISC up into internal vs user. https://github.com/netlify/edge-bundler/pull/347/files

Let's first get that merged then!

[danez]

This slightly confused me, until i figured that this is the displayName and the passed in name is the functionName. Could we make this more clear? maybe alias the input name as functionName or add a comment here that this is the display name

[khendrikse]

No that's not the case, the name field is the displayname in all cases with Edge Functions

[danez]

so we could do

Suggested change

	name: declaration?.name || config.name,
	name,

here? I don't believe so. With input I mean the name variable on Line 193

[khendrikse]

Ohh yeah that makes sense

[khendrikse]

I changed the way I named it now

[danez]

Wouldn't this be the same as Object.keys(newFunctionConfig).length > 0?
We are just trying to not add an empty object, right?

[khendrikse]

For some reason, before that didn't work... but now it does! Nice catch!

[danez]

Suggested change

	Object.entries(functionConfig).reduce((acc, [key, value]) => {
	if (value && !(Array.isArray(value) && value.length === 0)) {
	return { ...acc, [key]: value }
	}
	return acc
	}, {} as EdgeFunctionConfig)
	Object.fromEntries(Object.entries(functionConfig).filter(([_, value]) => value && !(Array.isArray(value) && value.length === 0)));

this is totally subjective, but I'm not a big fan of reduce, unless it is really reducing something. So I would find this much more readable and probably less objects created, but up to you.

[khendrikse]

I think I would find this less readable because it hides a lot of logic and I need to read it a lot of times to understand what is happening. That's why I prefer the reduce option

[eduardoboucas]

nit: I find this function a bit hard to follow. The name isn't super descriptive — it says it creates function configs but it's iterating one of the parameters and calling two other functions inside. I feel like an abstraction like addGeneratorFallback, which is simply adding a field, is adding to the confusion more than the benefits it brings.

[eduardoboucas]

What's "after a while"? Should this TODO be an issue instead?

[eduardoboucas]

nit: You could early-return and avoid nesting all these statements.

if (!excludedPath) {
  return
}