Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: move excluded_patterns into function_config #274
feat: move excluded_patterns into function_config #274
Changes from all commits
4807def
5f8c135
0fa20a9
722fe87
7c49753
520a241
cc1aa18
699a053
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this bit. Why are we suddenly setting the
cache
property specifically rather than all ofconfig
? And does it mean that if we add more configuration properties to ISC, will we have to add them here in addition toconfig.ts
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because
config
now not only containscache
andpath
, but alsoexcludedPatterns
, which should not be part of thedeclaration
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ^^