-
Notifications
You must be signed in to change notification settings - Fork 338
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: firefox binary path not passed in the run method exported by 'src/firefox/index' #468
fix: firefox binary path not passed in the run method exported by 'src/firefox/index' #468
Conversation
6bc8846
to
48bbd7b
Compare
@@ -333,6 +333,6 @@ export class ExtensionRunner { | |||
|
|||
run(profile: FirefoxProfile): Promise<FirefoxProcess> { | |||
const {firefoxApp, firefox} = this; | |||
return firefoxApp.run(profile, {firefox}); | |||
return firefoxApp.run(profile, {firefoxBinary: 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.
does this need to pass null
to satisfy the flow type or something? Could it just be {firefoxBinary: firefox}
?
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.
@kumar303 yep, it was a small leftover.
Removed in the updated patch.
r+wc (with comments). I just had one question about passing |
…c/firefox/index'.
48bbd7b
to
2ecc55e
Compare
@kumar303 the change suggested in the above comment has been applied and this pull request updated accordingly. |
Thanks! |
This PR fixes an issue with the optional Firefox binary path option and contains some tweaks to the related declared flow types, which shows how we can force flow to raise errors on this kind of issues (missing optional parameters in the
parameters
andoptions
objects)