-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: remove --config-file false references and update types #20643
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
…file-false-cleanup
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.
Cool PR, functionality seems fine, I left a bunch of comments - I think we have some quick wins in terms of code quality here, some related to your PR, some not. If you don't think this is the time and place to do them, all good, let me know and I can give you an approval.
Main comment I'd like to see at least considered/responded to: https://github.com/cypress-io/cypress/pull/20643/files#diff-aa4d6453f80b1103477a0cc93c94f94c5cfb3866d029b1b70708e77be023600bR82
@@ -2,7 +2,7 @@ export interface CommonModeOptions { | |||
invokedFromCli: boolean | |||
userNodePath?: string | |||
userNodeVersion?: string | |||
configFile?: false | string | null | |||
configFile?: 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.
How come this can be null
? I wonder if we can do configFile?: string
here. Not your change specifically but always good to question and improve code quality.
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.
It can be null here since you can opt to not pass in any value for configFile
and if none is passed, we will assume you are using the default and populate it with the detected file. So after ProjectLifeCycleManager runs, configFile
will be a string.
User facing changelog
Remove
--config-file=false
references and throw error if it is passedAdditional details
There are a few places we can throw this error, but I deemed it best to throw early so I could clean up some of the code that had special handling for
config-file=false
in the cli and server packages and to not spin up any further processes needlessly.Latest Cypress docs for unification have already been updated to remove config-file=false references.
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?