Skip to content
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(typedefs): Add type for autoEnd on LogConfig. #14220

Closed
wants to merge 2 commits into from

Conversation

linkel
Copy link

@linkel linkel commented Dec 17, 2020

User facing changelog

Add type for autoEnd on LogConfig.

Additional details

I wanted to make a small contribution to Cypress and get the dev environment set up. The only change I made was adding the type.

Side notes about Cypress development:

After I made the change, I built the package and built the binary, and in my test project I pointed to the package in package.json and had to copy paste the binary into my cypress cache in order to successfully yarn on my test project, otherwise it would try to download the binary online and fail since the version didn't exist.

I also had to downgrade my node version to avoid a build error in node-sass.

How has the user experience changed?

Now it doesn't produce a type error and shows autoEnd correctly typed.

image

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 17, 2020

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2020

CLA assistant check
All committers have signed the CLA.

@linkel
Copy link
Author

linkel commented Dec 17, 2020

Are the three circleci failures expected?

@jennifer-shehane
Copy link
Member

@linkel Yah those are some flaky tests.

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a comment here explaining what autoEnd is for, like the other props have.

I think this is only used to set autoEnd: false for .pause() command correct? The idea is that the command doesn't 'end' exactly.

Maybe understanding your use case for why you need to use this prop would be helpful.

@jennifer-shehane
Copy link
Member

@linkel Do you still have time to address the changes requested in this PR? We will have to close this PR due to inactivity otherwise.

@@ -5233,6 +5233,7 @@ declare namespace Cypress {
interface LogConfig extends Timeoutable {
/** The JQuery element for the command. This will highlight the command in the main window when debugging */
$el: JQuery
autoEnd: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a typedoc for this?

@emilgoldsmith
Copy link
Contributor

#15076 was now merged so this should be able to be closed

@bahmutov
Copy link
Contributor

closing as duplicate

@bahmutov bahmutov closed this Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'autoEnd' does not exist in type 'Partial<LogConfig>'
6 participants