fix: revert slash validation and change validation message

[danez]

Adjust the validation message as proposed in #339

[danez]

For paths coming from netlify.toml we already have validation

11:32:34 AM: Failed during stage 'Reading and parsing configuration files': 
When resolving config file /opt/build/repo/netlify.toml:
Configuration property edge_functions[1].path must be a valid path.

Invalid syntax

  [[edge_functions]]
  path = "hello/*"

Valid syntax

  [[edge_functions]]
  path = "/hello"
  function = "hello"

[eduardoboucas]

Same as above.

Suggested change

	'excluded_patterns must be an array of regex that starts with ^/ and ends with $ for example ^/blog/[d]{4}$',
	'excluded_patterns must be an array of regex that starts with ^/ and ends with $ (e.g. ^/blog/[d]{4}$)',

[klavavej]

+1 to @eduardoboucas's suggestion here too. Though I do want to re-ask my open questions from #339 (comment) because there are some discrepancies between this error message and a related docs update and I'd like to figure out how to resolve that

1. Is excluded_patterns an internal syntax or is this the property that users and frameworks should be using? I ask because the excluded patterns demo that I referenced while documenting exclusions uses the syntax excludedPattern (camel case singular vs snake case plural)

2. Are customers/frameworks able to enter an array of regexes for excludedPattern or is the support for arrays here an internal implementation detail (maybe for handling multiple declarations for the same function)? If customers/frameworks can enter an array of regexes what is the syntax for doing that in netlify.toml and manifest.json? I ask because the excluded patterns demo that I referenced while documenting exclusions didn't indicate support for arrays. Here's a table of my understanding of what is supported for a single declaration in the various formats (this understanding is what my open docs PR is based on https://github.com/netlify/docs/pull/2943). Please correct anything that I've got wrong

	inline Config	netlify.toml	manifest.json
path	string or array of strings	single string	single string
excludedPath	string or array of strings	single string	single string
pattern	not supported	single regex	single regex
excludedPattern	not supported	single regex	single regex

[danez]

1. The config option excludePattern is only allowed in conjunction with pattern according to our types in contrast to excludePath which only comes in conjunction with path. These are the names of the input options. In the manifest that edge-bundler produces (and which we validate) only pattern and exclude_patterns exist, but these are not options that anyone sets manually or any integration uses.
2. as it stands right now the table is

	inline Config	netlify.toml	manifest.json
path	string or array of strings	single string	string or array of strings
excludedPath	string or array of strings	not supported (1)	string or array of strings
pattern	not supported	not supported (1)	regex or array of regex
excludedPattern	not supported	not supported (1)	regex or array of regex

I believe this is not quiet correct and at least we should add excludedPath support to netlify.toml, not sure if pattern should be also supported? cc @eduardoboucas

Here are all the Typescript types for all 3 ways

Inline:

type Cache = "off" | "manual";
type Path = `/${string}`;
interface Config {
  cache?: Cache;
  path?: Path | Path[];
  excludedPath?: Path | Path[];
}

TOML:

type Cache = "off" | "manual";
type Path = `/${string}`;
interface Config {
  path: Path;
  function: string;
  cache?: Cache;
}

manifest.json:

type Cache = "off" | "manual";
type Path = `/${string}`;

interface Config {
  cache?: Cache
  function: string
  name?: string
  generator?: string
  path: Path | Path[]
  excludedPath?: Path | Path[]
}

// or

interface Config {
  cache?: Cache
  function: string
  name?: string
  generator?: string
  pattern: string | string[]
  excludedPattern?: string | string[]
}

[danez]

@eduardoboucas should excludePath be allowed in netlify.toml or is there a reason it is not there?

[klavavej]

+1 to @eduardoboucas's suggestion here too. Though I do want to re-ask my open questions from #339 (comment) because there are some discrepancies between this error message and a related docs update and I'd like to figure out how to resolve that

1. Is excluded_patterns an internal syntax or is this the property that users and frameworks should be using? I ask because the excluded patterns demo that I referenced while documenting exclusions uses the syntax excludedPattern (camel case singular vs snake case plural)

2. Are customers/frameworks able to enter an array of regexes for excludedPattern or is the support for arrays here an internal implementation detail (maybe for handling multiple declarations for the same function)? If customers/frameworks can enter an array of regexes what is the syntax for doing that in netlify.toml and manifest.json? I ask because the excluded patterns demo that I referenced while documenting exclusions didn't indicate support for arrays. Here's a table of my understanding of what is supported for a single declaration in the various formats (this understanding is what my open docs PR is based on https://github.com/netlify/docs/pull/2943). Please correct anything that I've got wrong

	inline Config	netlify.toml	manifest.json
path	string or array of strings	single string	single string
excludedPath	string or array of strings	single string	single string
pattern	not supported	single regex	single regex
excludedPattern	not supported	single regex	single regex

[klavavej]

If this is meant to be a customer-facing error message, I think it would be helpful to use the customer-facing input name rather than the name of the internal property we validate. Using the customer-facing name should help folks find and fix the invalid value that's triggering the error message.

Suggested change

	'excluded_patterns must be an array of regex that starts with ^/ and ends with $ (e.g. ^/blog/[d]{4}$)',
	'excludedPattern must be an array of regex that starts with ^/ and ends with $ (e.g. ^/blog/[d]{4}$)',

[danez]

Hmm.. The worst part about this validation is that it will most likely not be fixable by the user.

1. If a user misses the slash in netlify.toml the config validation at the beginning of the build will fail with an error message.

11:32:34 AM: Failed during stage 'Reading and parsing configuration files': 
When resolving config file /opt/build/repo/netlify.toml:
Configuration property edge_functions[1].path must be a valid path.

Invalid syntax

  [[edge_functions]]
  path = "hello/*"

Valid syntax

  [[edge_functions]]
  path = "/hello"
  function = "hello"

: exit status 1

2. If the slash is missing in the in-source-config then TypeScript will warn about that.

So it is pretty unlikely that this error comes from invalid user input, but more likely from an integration that generates a pattern without a slash. So I think a user will in 90% not know what to do, no matter how we name it.

I wonder if we should in addition of validating the manifest have validation for edge function declarations from integrations, so that we can point the user to an integration was declaring an invalid edge function

[klavavej]

I wonder if we should in addition of validating the manifest have validation for edge function declarations from integrations, so that we can point the user to an integration was declaring an invalid edge function

I like this idea! This could give the user a more actionable step forward (remove the integration or contact the integration provider with a bug report). It would be great if an error message like this could provide generator info for the problematic function to help users figure out which integration to remove / contact with a bug report.

[danez]

We decided to not validate the leading slash in pattern and excludePattern, as there might be cases where there are valid reasons not to start with slash like ^(?:/path1|/path2)$ and there is also not real gain for us nor for the integrator to enforce this check.

This means that in this PR I reverted the FF for the check and removed the actual check again. The new message is still valid, just without the slash.

[klavavej]

revised error message looks good to me

meant to submit the latest review as a comment, not as a blocking "request changes" review