-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix: Cannot add commands with "reserved" names #18923
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: Cannot add commands with "reserved" names #18923
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 |
…/github.com/cypress-io/cypress into issue-6146-prevent-adding-reserved-commands
getAlias
, reset
@davidmunechika Do you have a screenshot of the new error & stack trace? |
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.
Here's a way to reproduce an issue with cy.reset
:
Cypress.Commands.add('reset', () => {})
it('a test', () => {})
It errors even though the user hasn't called the cy.reset()
command they created, because we use it internally.
We need to do this for every method/property we add internally to cy
. If they "add" a command with that name, it's overriding the functionality of that method and can cause issues. It might take a certain set of circumstances to reproduce an issue, but those methods are relied on somewhere in the codebase, so we can't allow them to be overwritten.
Really, we shouldn't have so many utility methods attached to cy
. I think most of them are on there for convenience's sake. I would say we shouldn't have any undocumented methods on cy
. At least they should be prefixed with an underscore or under a namespace that makes it clear they're for internal use. Something that reduces the number of unusable command names. Even better would be to refactor them out into more appropriate places and reduce the need for a massive object of utility methods. But this should be a separate discussion and effort.
For now and for this PR, I think it makes sense to add all these methods/properties to the list of reserved names and error if a user tries to use them. In the current state of the world, they'll cause unexpected behavior if overridden. Then if we refactor them off of cy
in the future, they can be removed from the list of reserved names.
Co-authored-by: Chris Breiding <[email protected]>
…/github.com/cypress-io/cypress into issue-6146-prevent-adding-reserved-commands
I changed the base branch to 10.0-release, but I'm sure how well that will resolve. It's possible it would be easier to create a new branch based off 10.0-release and cherry-pick the changes into it. |
User facing changelog
Attempting to add a new command using
Cypress.Commands.add()
that has the same name as an internal function reserved by Cypress will show an improved error message explaining why the command name must be changed.Additional details
Previously users would encounter strange behavior when trying to add new custom commands with the same name as a reserved internal function by Cypress (i.e. TypeError...). This change improves the error message to explain that users cannot name new custom commands as the same name as an internal function already reserved by Cypress.
Old behavior:
New error and stacktrace:
PR Tasks
cypress-documentation
? Adding an existing command or reserved internal function withCypress.Commands.add()
will throw an error cypress-documentation#4178