-
Notifications
You must be signed in to change notification settings - Fork 903
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
adds filterExcept to normalizedHostingConfigs #3593
Conversation
@@ -56,6 +56,41 @@ function filterOnly(configs: HostingConfig[], onlyString: string): HostingConfig | |||
return filteredConfigs; | |||
} | |||
|
|||
function filterExcept(configs: HostingConfig[], exceptString: string): HostingConfig[] { |
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.
Nit: exceptString
seems like a redundant name since we already declare its type. Prefer just except
instead.
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.
In context, it's not important that it's a "string" by the name, but that it's the "option string" from the CLI options object. I'll go with exceptOption: string
to make it clear (it's used quickly and briefly, so that should work).
return []; | ||
} | ||
|
||
const exceptSet = new Set( |
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.
Nit: Don't love this name either - its redundant with the type, and it doesn't convey that this is only the hosting excepts. Consider something like hostingExcepts
instead?
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.
Done.
exceptTargets.filter((t) => t.startsWith("hosting:")).map((t) => t.replace("hosting:", "")) | ||
); | ||
|
||
const configsBySite = new Map<string, HostingConfig>(); |
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.
These are unused, no? it looks like you just add configs to these and then forget about them...
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're right. I added the set to get rid of these and didn't get rid of them
}, | ||
]; | ||
|
||
for (const t of tests) { |
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've seen this pattern for testing in the backend but never in the CLI before - I'm totally stealing this :)
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 KNOW RIGHT?!
* adds filterExcept to normalizedHostingConfigs fixes firebase#3397 * remove .only * add changelog * pass only'd configs to except * one clarifying comment * rename and cleanup
Description
Adds support for
--except
when deploying to Firebase Hosting.fixes #3397
Scenarios Tested
other
is the target forOTHER-SITE
. I also have some functions...