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

[CLEANUP beta] Remove jQuery usage from ember-testing #19675

Merged
merged 1 commit into from Aug 18, 2021

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Jul 22, 2021

As advised by feedback from core team, this removes all jQuery related testing helpers from ember-testing, along with related utilities (event stuff) and tests:

  • find
  • findWithAssert
  • triggerEvent
  • keyEvent
  • click
  • fillIn

All other helpers remain untouched for future official deprecation.


This is meant as a prerequisite to further jQuery cleanup, like removing the internal jQueryDisabled flag.

Basically I am assuming that in 4.0 jQueryDisabled is implicitly always true (similar to assuming JQUERY_INTEGRATION is always false). So here I am basically doing manual dead code elimination. However that means:

  • ember-testing's find() is always throwing an assertion now, and as such probably most other helpers that internally use find()?
  • some test suites have been completely removed, as all of it was wrapped in if (!jQueryDisabled) { ... } (so under that above assumption that is if (false) {} now)

Note sure how this whole package was meant to migrate to 4.0, so please provide advice. AFAIK the legacy testing API's (before @ember/test-helpers) weren't formally deprecated, were they? But nowadays without jQuery, they seem not even usable anymore?

/cc @rwjblue

@simonihmig simonihmig force-pushed the jquery-ember-testing branch 2 times, most recently from 8efcb80 to 6e00ef3 Compare July 22, 2021 20:08
@simonihmig simonihmig marked this pull request as draft July 22, 2021 20:09
@mixonic
Copy link
Sponsor Member

mixonic commented Aug 1, 2021

Hm, @simonihmig I believe the testing APIs should be removed if they are now always erring. They are replaced by https://github.com/emberjs/ember-test-helpers.

If that is acceptable for API that error, I think another question is what happens to all the non-erroring helpers offered in the same manner. Those might need deprecation targeting 5.0 removal. I'll add this to our framework discussion Friday.

@mixonic
Copy link
Sponsor Member

mixonic commented Aug 14, 2021

@simonihmig We discussed this with framework core. The group agreed that any testing APIs that were reliant on jQuery should simply be removed in 4.0. If there are testing APIs that do not reference jQuery, they should remain. Separately we could work on deprecating those, but it would target 5.0 at this point.

@simonihmig
Copy link
Contributor Author

The group agreed that any testing APIs that were reliant on jQuery should simply be removed in 4.0. If there are testing APIs that do not reference jQuery, they should remain.

@mixonic Did that now! See the PR's updated description. Ready for review then!

@simonihmig simonihmig marked this pull request as ready for review August 17, 2021 15:48
@simonihmig simonihmig changed the title Remove jQuery usage from ember-testing [CLEANUP beta] Remove jQuery usage from ember-testing Aug 17, 2021
Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

This is wonderful!!

As advised by feedback from core team, this removes all jQuery related testing helpers from ember-testing:
* `find`
* `findWithAssert`
* `triggerEvent`
* `keyEvent`
* `click`
* `fillIn`

All remaining helpers remain untouched for future official deprecation.
Copy link
Sponsor Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

I pushed an amended PR which drops changes to the yarn.lock file that I believe were unrelated.

@mixonic mixonic merged commit 747f5ff into emberjs:master Aug 18, 2021
@simonihmig simonihmig deleted the jquery-ember-testing branch August 18, 2021 08:02
@simonihmig
Copy link
Contributor Author

I pushed an amended PR which drops changes to the yarn.lock file that I believe were unrelated.

Oh, didn't notice, thanks! I believe that happened to me more than once, idk why, maybe different versions of yarn?
I could create a PR to add the volta config to package.json, so at least volta users would use the same deterministic versions, does that make sense?

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.

None yet

3 participants