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

Support @typescript-eslint/no-unused-vars rule #502

Closed
wants to merge 1 commit into from
Closed

Support @typescript-eslint/no-unused-vars rule #502

wants to merge 1 commit into from

Conversation

karlhorky
Copy link

@karlhorky karlhorky commented Jul 22, 2021

User facing changelog

This disables the ESLint @typescript-eslint/no-unused-vars rule (the typed counterpart to ESLint's built-in no-unused-vars)

Additional details

The files scaffolded by Cypress create an error out of the box for those users who are using @typescript-eslint/no-unused-vars instead of no-unused-vars

This rule is also possible (and desirable) to use with JS files as well - it offers additional type information even in JS files so that the rule catches more problems.

How has the user experience changed?

Before: Error out of the box

Screen Shot 2021-07-21 at 22 02 48

After: No error

Screen Shot 2021-07-21 at 22 03 34

Original PR: cypress-io/cypress#17432

cc @flotwig

@CLAassistant
Copy link

CLAassistant commented Jul 22, 2021

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Jul 22, 2021

✔️ Deploy Preview for cypress-example-kitchensink ready!

🔨 Explore the source changes: 7913c0a

🔍 Inspect the deploy log: https://app.netlify.com/sites/cypress-example-kitchensink/deploys/60f9e9d51497f80008c203e5

😎 Browse the preview: https://deploy-preview-502--cypress-example-kitchensink.netlify.app

@cypress
Copy link

cypress bot commented Jul 22, 2021



Test summary

5 0 0 0


Run details

Project cypress-example-kitchensink
Status Passed
Commit 7913c0a
Started Jul 22, 2021 9:59 PM
Ended Jul 22, 2021 10:02 PM
Duration 03:37 💡
OS Linux Ubuntu - 16.04
Browser Custom chromium 90

View run in Cypress Dashboard ➡️


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

@MikeMcC399
Copy link
Contributor

@emilyrohrbough

"This branch has conflicts that must be resolved".

This PR changes a file which no longer exists under Cypress 10+

Did you approve it by accident? Can we check that the issue it was addressing still exists?

@MikeMcC399
Copy link
Contributor

cypress/plugins/index.js is created by default after starting Cypress 9.7.0 with

// eslint-disable-next-line no-unused-vars
module.exports = (on, config) => {
  // `on` is used to hook into various events Cypress emits
  // `config` is the resolved Cypress config
}

The file remains after updating to Cypress 12.9.0 and then running the migration wizard.

After a clean installation of Cypress 12.9.0, opening this version and running E2E scaffolding, no file cypress/plugins/index.js is created. There is no other file with the above problematic code snippet.

@MikeMcC399
Copy link
Contributor

The issue targeted by this PR is obsolete and the PR can be closed. There is nothing to fix anymore.

Modifications to examples in this repo (cypress-io/cypress-example-kitchensink) only flow into future versions of Cypress. They do not affect released versions or existing installations.

The issue only affects installations of Cypress Legacy configuration: Folders/files which were originally installed using Cypress 9 and lower.

Installations using Cypress current Configuration: Folders/files which were originally installed using Cypress 10+ are not affected. There is no file cypress/plugins/index.js created.

@MikeMcC399
Copy link
Contributor

This PR should be closed because it is obsolete, but also because it would not have worked even if it had been dealt with in a timely manner.

Perhaps @karlhorky or one of the Cypress.io team could close it?

Rebasing

Attempting to rebase the branch https://github.com/karlhorky/cypress-example-kitchensink/tree/patch-2 on the master branch results in a conflict:

$ git rebase master pr/502
CONFLICT (modify/delete): cypress/plugins/index.js deleted in HEAD and modified in 7913c0a (Support @typescript-eslint/no-unused-vars rule).  Version 7913c0a (Support @typescript-eslint/no-unused-vars rule) of cypress/plugins/index.js left in tree.
error: could not apply 7913c0a... Support @typescript-eslint/no-unused-vars rule
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 7913c0a... Support @typescript-eslint/no-unused-vars rule

Since there is no other change in the PR apart from a modification to the deleted file cypress/plugins/index.js, after resolving the conflict, there is nothing left in the PR.

Effectiveness

It seems also that there was a misconception that changing cypress/plugins/index.js in this repository would affect the file delivered by installing and running a legacy Cypress version (9 or earlier). In fact although this file was packaged up and included in the npm module cypress-example-kitchensink, the Cypress build process described in Updating the example app using yarn workspace @packages/example build did not copy this file. See

https://github.com/cypress-io/cypress/blob/d681fd929bfeb4ca297d42ad77fc3d0e33dae793/packages/example/bin/convert.js#L44-L50

   glob('./cypress/integration/**/*.js', { realpath: true }, (err, specFiles) => {
    if (err) throw err

    htmlFiles.concat(specFiles).forEach(function (file) {
      return replaceStringsIn(file)
    })
  })

So even if Cypress were still using the layout which is now legacy, the PR would not have had the desired effect.

@karlhorky
Copy link
Author

Yeah I'm thinking also that this PR is no longer needed.

If I see this happening in the latest versions, I'll open a new PR.

@karlhorky karlhorky closed this May 3, 2023
@karlhorky karlhorky deleted the patch-2 branch May 3, 2023 07:45
@MikeMcC399
Copy link
Contributor

@karlhorky

Thanks for closing! I'm going through some of the eslint aspects at the moment, so please flag any issues if you find any new ones.

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.

5 participants