-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[@now/build-utils] Add support for includeFiles
and excludeFiles
to Functions
#3285
Conversation
…tils/support-include-files
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.
Looks very good!
@@ -308,6 +313,24 @@ function validateFunctions(files: string[], { functions = {} }: Options) { | |||
}; | |||
} | |||
} | |||
|
|||
if (func.includeFiles !== undefined) { | |||
if (!Array.isArray(func.includeFiles) || !func.includeFiles.every(file => typeof file === 'string')) { |
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.
You are missing the scenario when includeFiles
is a string.
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.
@styfle @leo I don't think we need to support that case here though, I'd rather just have one type instead of two types, which will be translated internally to the same one anyways.
I also don't think it's too bad if someone want's to migrate from builds
to functions
and just needs to wrap the string around [
and ]
.
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.
If we want to make a breaking change here, lets go with a single string
instead of an array.
Because the user can implement a glob that matches multiple directories like {templates/**,views/**}
. This should yield better performance because we wouldn't need to read the filesystem multiple times.
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.
Agreed!
We're currently enforcing the same across our new project settings. If the field has support for globs, it should be a string, since glob already adds support for multiple matches.
Add support for
includeFiles
andexcludeFiles
to Functions.PRODUCT-27