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

Handle ImportBindings in no-unnecessary-waiting rule #109

Conversation

walaszczykm
Copy link
Contributor

@walaszczykm walaszczykm commented Sep 7, 2022

This PR adds proper handling for ImportBindings in no-unnecessary-waiting rule to prevent reporting codes like this:

// We don't know if WAIT is a number or alias '@someRequest'
import { WAIT } from './constnats'

cy.wait(WAIT)

It's similar to the existing case with function arguments where we cannot see the type of the argument. Similarly, we cannot know the type of import binding.

  // function wait (amount) { cy.wait(amount) }
  // we can't know the type of value, so don't fail
  if (!param || param.type !== 'AssignmentPattern') return false

Resolves #43

@spencer-vizio
Copy link

@walaszczykm Any update if this can get merged? I deal with this constantly :(

@walaszczykm
Copy link
Contributor Author

@spencer-vizio I would love to! 🙌 Unfortunately, I am not a core contributor to this project, and it looks like the conversation on the issue lost track after my comment and PR submission 😢

@klarzynskik
Copy link

Hey @nagash77 or @chrisbreiding, maybe you would be able to help with merging this or advise? 🙏🏼 Apologies for the direct mention, but this one seems ready to go. 🤞🏼

@CLAassistant
Copy link

CLAassistant commented Apr 10, 2023

CLA assistant check
All committers have signed the CLA.

@emilyrohrbough
Copy link
Member

@walaszczykm can you please sign the CLA?

@walaszczykm
Copy link
Contributor Author

@emilyrohrbough Sure signed! ✅ thanks for taking a look at this 🙂

@emilyrohrbough
Copy link
Member

@walaszczykm I am reading this change, and I am not sure this is a valid change to this rule. This eslint rule was written to error/warn when unnecessary waits for specific hard-coded number is sued and this change would allow this to "passed" when really this isn't always necessary.

Is this removing the validity of the rule or is this change just accounting for/correctly linked values that are imported?

@walaszczykm
Copy link
Contributor Author

walaszczykm commented Apr 11, 2023

@emilyrohrbough

This eslint rule was written to error/warn when unnecessary waits for specific hard-coded number is sued and this change would allow this to "passed" when really this isn't always necessary.

I am mainly referring to this existing condition in the rule's code:

  // function wait (amount) { cy.wait(amount) }
  // we can't know the type of value, so don't fail
  if (!param || param.type !== 'AssignmentPattern') return false

If we can not be sure about the type of value IMO the rule shouldn't try to force it as a bad practice and simply make it don't fail. This is exactly the same case with import bindings like it is with function augments that don't have default values. I can add a similar reasoning argument as a code comment near added line so it would be more clear.

I have rerun the test, and yes, the example in the PR is properly reported so I will rewrite the PR description to don't make confusion and refer to the case with function arguments plus add a code comment 👍

@walaszczykm walaszczykm force-pushed the no-unnecessary-waiting-support-import-bindings branch from ea34a74 to 3997499 Compare April 11, 2023 07:47
@mjhenkes mjhenkes assigned mjhenkes and unassigned emilyrohrbough Apr 11, 2023
@mjhenkes mjhenkes assigned mschile and unassigned mjhenkes Apr 27, 2023
@mschile mschile merged commit c626ad5 into cypress-io:master Apr 28, 2023
@mschile
Copy link
Contributor

mschile commented Apr 28, 2023

Thanks for the contribution @walaszczykm and sorry for the delay in getting this PR merged!

@nagash77
Copy link
Contributor

🎉 This PR is included in version 2.13.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-unnecessary-waiting: TypeError: Cannot read property 'null' of undefined
9 participants