-
Notifications
You must be signed in to change notification settings - Fork 57
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
fix: use target
from webExtConfig if browser
option is undefined
#232
Conversation
: "chromium"; | ||
pluginOptions.browser === "chrome" | ||
? "chromium" | ||
: pluginOptions.browser === null || |
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 wonder if we should deprecate the browser: null
setting. It's a bit cryptic 😅
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.
Huh, yeah, wonder what I was thinking lol. Let's leave it for now.
browser?: string; | ||
browser?: 'chrome' | 'firefox' | null; |
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 should remain a string
. You can target more than just chrome or firefox. Safari, opera, etc.
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.
Oops! That's strictly for the manifest templating? I don't see any handling for Safari and others in createWebExtRunner
, hence the assumption. 😄
@@ -62,7 +62,7 @@ export interface UserOptions { | |||
* | |||
* @default "chrome" | |||
*/ | |||
browser?: string; | |||
browser?: 'chrome' | 'firefox' | (string & {}) | null; |
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 a TypeScript trick that provides auto-completion for some values while still allowing any 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.
Interesting, didn't know this worked... I've only used branded types with a property inside the {}
. I've always had to use a function overload to get this behavior before, very good to know!
Based on #231 so merge that first.
If the
browser
plugin option is undefined, check thewebExtConfig.target
option before defaulting to chromium.