Add implementation-defined maximums #332
Conversation
apasel422
left a comment
apasel422
left a comment
There was a problem hiding this comment.
The spec and simulator changes here LGTM, though I'll defer to others on the choice of the exact minimum values mandated by the spec.
| "maxConversionSitesPerImpression": 3, | ||
| "maxConversionCallersPerImpression": 3, | ||
| "maxImpressionSitesForConversion": 3, | ||
| "maxImpressionCallersForConversion": 3, |
There was a problem hiding this comment.
I know setting lower values here simplifies test construction, but it may be confusing to anyone stumbling across this code for them to be less than the minimums mandated by the specification. I'm OK with keeping them this low for now, but we might want to add a $comment noting this.
| limit: number, | ||
| ): Set<string> { | ||
| const parsed = new Set<string>(); | ||
| if (limit !== null && input.length > limit) { |
There was a problem hiding this comment.
| if (limit !== null && input.length > limit) { | |
| if (input.length > limit) { |
The limit !== null condition is redundant with limit: number above, which makes the value non-nullable.
| /// The maximum number of conversion callers per impression. | ||
| readonly maxConversionSitesPerImpression: number; | ||
| /// The maximum number of conversion sites per impression. | ||
| readonly maxConversionCallersPerImpression: number; |
There was a problem hiding this comment.
| /// The maximum number of conversion callers per impression. | |
| readonly maxConversionSitesPerImpression: number; | |
| /// The maximum number of conversion sites per impression. | |
| readonly maxConversionCallersPerImpression: number; | |
| /// The maximum number of conversion sites per impression. | |
| readonly maxConversionSitesPerImpression: number; | |
| /// The maximum number of conversion callers per impression. | |
| readonly maxConversionCallersPerImpression: number; |
| const maxMatchValues = this.#delegate.maxMatchValues; | ||
| if (matchValues.length > maxMatchValues) { | ||
| throw new RangeError( | ||
| `matchValues size must be in the range [0,${maxMatchValues}]`, |
There was a problem hiding this comment.
| `matchValues size must be in the range [0,${maxMatchValues}]`, | |
| `number of values in matchValues exceeds limit of ${maxMatchValues}`, |
To match the error format for parseSites, and because there can't be fewer than 0 entries in the list.
| }, | ||
| "maxConversionSitesPerImpression": 3, | ||
| "maxConversionCallersPerImpression": 3, | ||
| "maxImpressionSitesForConversion": 3, |
There was a problem hiding this comment.
We will have to update impl/e2e.schema.json as well with the new configuration fields.
This includes a bunch of values that I essentially made up. Rationale for that is listed in #174.
We'll have to discuss what appropriate values are
before we can merge this change.
Closes #174.
Preview | Diff