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

check that item.first function exists before calling #3084

Merged
merged 7 commits into from Mar 15, 2019
Merged

Conversation

jennifer-shehane
Copy link
Member

- should help with support of jQuery legacy
- fix issue introduced by
#2870
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

  • needs a test, can likely use an old version of jquery as a fixture
  • needs to add comments above the code referencing this issue so we understand why it's necessary

@jennifer-shehane
Copy link
Member Author

So, I haven't been able to directly replicate this with the standard jQuery 2.2.4, but I've been able to replicate with the user's example code - which includes commonjs, so it's a bit of a frakenstein script, which I'd rather not do, but I added a test with this JS script if you can review and see anything problematic with it.

But, I also think it's just common sense to check the existence of something before calling a function on it, so how harmful can the change be (famous last words).

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

deleted

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

There are two primary problems:

  1. this utils function must return the first DOM element in the jquery collection. simply not calling item.first() will cause failures in other ways since this function is no longer doing what it's supposed to be doing
  2. because this PR does not replicate the actual problem, thats why all the tests are passing. we must have a test that causes the thing we're fixing to actually break

There's also one secondary problem:

  1. you shouldn't commit packages/driver/test/cypress/fixtures/jquery-2.2.4-common.js to source control. I see that jquery-3.2.1.js is there, but that shouldn't be committed either. The driver can use a utility called npm-install-version to install multiple versions of the same library into node_modules which can then be used by the .html fixtures and avoiding the need for source control. We do this in packages/driver/test/support/server.coffee for react and it needs to be done for jquery as well - and all fixtures need to be updated to point to the specific version of jquery it needs.

## Need check to support jQuery legacy versions
## https://github.com/cypress-io/cypress/issues/2927
if item.first
return item.first()
Copy link
Member

Choose a reason for hiding this comment

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

We can very simply re-wrap the jquery object in our own controlled jquery object before calling .first()

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we pair on this in our meeting today?

@agon21
Copy link

agon21 commented Feb 22, 2019

@jennifer-shehane I am getting one of my tests failing due to this error too. item.first is not a function

Any planned date to release this patch on 3.1.6?
Thanks

@brian-mann brian-mann merged commit 0e3289d into develop Mar 15, 2019
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.

Cypress.io 3.1.3 breaks by legacy jQuery library
3 participants