remove jquery from ember-testing #16075

snewcomer · 2018-01-06T06:10:58Z

Edited: The path forward now is to warn the end user that if jQuery is disabled, they should use instead @ember/test-helpers.

References #16058

ro0gr · 2018-01-06T19:46:40Z

If we force the user to refactor

I believe we can't do this. At least I don't know any RFC with this discussed.
I think we have to preserve jquery support in the ember-testing package till the end of its days.

@rwjblue isn't @ember/test-helpers supposed to replace ember-testing package eventually?

snewcomer · 2018-01-06T20:52:51Z

Another potential hairy spot - Listening for ajaxSend/ajaxComplete and triggering that event are easy with JS, but I believe one difficulty is getting all the event listeners here (across multiple browsers) with JS. jQuery makes this nice and easy. JS equivalent is getEventListeners, but I think that only works w/ Chrome Firefox. && Safari.

Could create our own data structure to hold these events instead.... 🤔

snewcomer

Two items left include:

find - replace jquery with JS
Bring back those tests with some data structure that holds / resets ajaxSend && ajaxComplete

snewcomer · 2018-01-07T20:38:27Z

packages/ember-testing/lib/events.js

-        } else {
-          $el.trigger('focusin');
-        }
+        let browserIsNotFocused = document.hasFocus && !document.hasFocus();


Looks like there is a better, but similar sol'n here

snewcomer · 2018-01-08T04:51:44Z

packages/ember-testing/lib/helpers/find_with_assert.js

  selector argument to find only within the context's children
-  @return {Object} jQuery object representing the results of the query
-  @throws {Error} throws error if jQuery object returned has a length of 0
+  @return {Object} DOM element representing the results of the query


This has yet to be implemented. find is still using jquery. If we merge w/o the find change, I can revert these files.

rwjblue · 2018-01-08T16:03:36Z

packages/ember-testing/lib/events.js

        // Firefox does not trigger the `focusin` event if the window
        // does not have focus. If the document doesn't have focus just
        // use trigger('focusin') instead.
+        if (browserIsNotFocused) {
+          fireEvent(el, 'focusin');


FWIW, I totally realize that this isn't a mistake in your migrate to native from jQuery, and that it was preexisting. During my testing of this functionality while migrating it to ember-test-helpers I don't think this is quite right, it should be something akin to this:

// makes `document.activeElement` be `element`. If the browser is focused, it also fires a focus event element.focus(); // Firefox does not trigger the `focusin` event if the window // does not have focus. If the document does not have focus then // fire `focusin` event as well. if (browserIsNotFocused) { // if the browser is not focused the previous `el.focus()` didn't fire an event, so we simulate it fireEvent(element, 'focus', { bubbles: false, }); fireEvent(element, 'focusin'); }

Otherwise the event ordering is incorrect...

See related implementation from @ember/test-helpers.

Getting these ie11 errors after refactoring to this.

https://travis-ci.org/emberjs/ember.js/jobs/326476166

Looks like it is triggering the events multiple times and causing pain click triggering events in order.

rwjblue · 2018-01-08T16:06:20Z

packages/ember-testing/lib/events.js

@@ -88,7 +96,8 @@ function buildKeyboardEvent(type, options = {}) {
  let event;
  try {
    event = document.createEvent('KeyEvents');
-    let eventOpts = jQuery.extend({}, DEFAULT_EVENT_OPTIONS, options);
+    let eventOpts = assign({}, DEFAULT_EVENT_OPTIONS);
+    assign(eventOpts, options);


Ember.assign is a rough polyfill for Object.assign and does properly accept multiple arguments. Can you update to:

let eventOpts = assign({}, DEFAULT_EVENT_OPTIONS, options);

rwjblue · 2018-01-08T16:10:04Z

packages/ember-testing/lib/helpers/fill_in.js

@@ -32,7 +32,7 @@ export default function fillIn(app, selector, contextOrText, text) {
  el = $el[0];
  focus(el);

-  $el.eq(0).val(text);
+  el.value = text;


Can you confirm if $el.eq(0).val(text) supported content editable elements? I don't recall if it did, but if it did then this would be a regression (since when content editable set you need to set innerHTML).

See related code from @ember/test-helper.

Seems that both jquery and JS. And yeah def need to set that innerHTML! 👍

el = document.createElement('div') $(el).attr('contenteditable', 'true') $(el).val('wat') $(el).text() // or $(el).html() // ""

rwjblue · 2018-01-08T16:12:24Z

packages/ember-testing/lib/support.js

-    .on('click', handler)
-    .trigger('click')
-    .remove();
+  input.setAttribute('type', 'checkbox');


I think this should stay back in jQuery mode since its only ran behind a !jQueryDisabled guard

rwjblue · 2018-01-08T16:15:06Z

packages/ember-testing/tests/helpers_test.js

-      'onInjectHelpers are called after injectTestHelpers'
-    );
-  }
+  // [`@test #setupForTesting attaches ajax listeners`](assert) {


Can you bring these tests back, but put a short circuit in when jQuery is disabled?

something like:

['@test asdfasdf'](assert) { if (jQueryDisabled) { assert.expect(0); return; } }

So this one might be a bit difficult to get at the event listeners w/ documentEvents = jQuery._data(document, 'events'); if jquery is enabled since it is now using document.addEventListener in setup-for-testing. The only thing I was thinking was to track the events with a data structure of our own. Thoughts?

snewcomer · 2018-01-08T18:36:58Z

packages/ember-testing/lib/helpers/find.js

@@ -29,6 +30,15 @@ import { get } from 'ember-metal';
 export default function find(app, selector, context) {
  let $el;
  context = context || get(app, 'rootElement');
-  $el = app.$(selector, context);
-  return $el;
+  if (jQueryDisabled) {


So to allow this to work in all cases, find needs to accept an element or selector...not just a selector.

rwjblue · 2018-01-08T18:39:41Z

packages/ember-testing/lib/helpers/find.js

-  return $el;
+  if (jQueryDisabled) {
+    let rootElement = document.querySelector(context);
+    $el = rootElement.querySelector(selector);


This should be returned, (we don't need to wrap in an array)

Also, I think we should do:

let result; if (context instanceof Element) { result = context.querySelector(selector); } else if (typeof context === 'string') { result = document.querySelector(`${context} ${selector}`); } else { result = document.querySelector(selector); } return result;

Fixed! 🌮

So to support both jquery and JS, I think we need to return an array or array-like object from find. Other helpers are expecting this (like findWithAssert). Unless we wanted to go with only returning one element; however, perhaps users were using find to select multiple as show here.

rwjblue · 2018-01-08T18:39:59Z

packages/ember-testing/lib/helpers/find.js

+    }
+    return [];
+  } else {
+    $el = app.$(selector, context);


Can probably return this directly

simonihmig · 2018-01-08T19:39:26Z

packages/ember-testing/lib/helpers/click.js

    // assert something
  });
  ```

  @method click
-  @param {String} selector jQuery selector for finding element on the DOM
-  @param {Object} context A DOM Element, Document, or jQuery to use as context
+  @param {String} selector css selector for finding element on the DOM


Although the functionality has not changed and still uses jquery when it's available, this docs change could be misleading, suggesting there is a breaking change. Shouldn't this state something like jQuery selector or css selector (if jQuery is not present) for finding element on the DOM?

Oh, seems you changed it already while I wrote this!? :)

That seems like a good option. I have it now as @param {String|Element} target the element or selector to click on. What do you think of this? Should it be:

the element or jQuery selector or css selector (if jQuery is not preset) to click on

Seems good to me

snewcomer

I think almost done! Lmk what you think about the conditionals I put around the tests.

snewcomer · 2018-01-08T22:48:59Z

packages/ember-testing/tests/helpers_test.js


-    setupForTesting();
+    // [`@test #setupForTesting attaches ajax listeners`](assert) {


If we want to bring back these tests, we would have to add back the jquery.on listeners in setup_for_testing. I don't think there is a way to track all of the events listening in a cross browser way (getEventListeners; however, I think no IE). We could add an explicit trackEventLIsteners method but I don't think we would want to do that either.

However, it seems to me that the below tests cover the functionality in setup_for_testing and these commented out tests could be deleted.

https://github.com/emberjs/ember.js/blob/master/packages/ember-testing/tests/helpers_test.js#L1074

@rwjblue lmk what you think about this remaining issue.

@snewcomer - Can you delete them?

snewcomer · 2018-01-09T03:39:39Z

packages/ember-testing/lib/helpers/click.js

@@ -17,7 +17,7 @@ import { focus, fireEvent } from '../events';

  @method click
  @param {String} selector jQuery selector for finding element on the DOM
-  @param {Object} context A DOM Element, Document, or jQuery to use as context
+  @param {Object} context A DOM Element, Document, or css to use as context


need to revert this

snewcomer · 2018-01-09T03:40:06Z

packages/ember-testing/lib/helpers/find_with_assert.js

@@ -21,8 +21,8 @@
  the DOM
  @param {String} [context] (optional) jQuery selector that will limit the
  selector argument to find only within the context's children
-  @return {Object} jQuery object representing the results of the query
-  @throws {Error} throws error if jQuery object returned has a length of 0
+  @return {Object} DOM element representing the results of the query


need to revert this.

snewcomer · 2018-01-09T03:41:08Z

packages/ember-testing/tests/integration_test.js

+      window.visit('/').then(() => {
+        expectAssertion(() => window.find('.name'), 
+        /If not using jQuery, please import and use methods from @ember\/test-helpers/);
+      });


This asserts that assert is thrown

rwjblue · 2018-01-11T23:29:21Z

I left one inline comment, and other than that we need to squash down the commits into one and this should be good to go...

ro0gr · 2018-01-11T23:53:32Z

packages/ember-testing/lib/helpers/find.js

  @public
 */
 export default function find(app, selector, context) {
+  if (jQueryDisabled) {
+    assert('If not using jQuery, please import and use methods from @ember/test-helpers');


which helper exactly can be used? I think @ember/test-helpers doesn't provide any alternatives to find.

@ro0gr - Usages of find should be replaced by this.element.querySelector or this.element.querySelectorAll...

but the old good acceptance tests don't have this.element. Should this assertion have a reference to setupTest api?

FWIW - By the time this hits a release version, the guides will all have been updated to reference the new style (work progressing in emberjs/guides#2145).

For now, lets update the assertion to tell the user that find is not available when jQuery is disabled without giving a more detailed recourse (we can tweak the message when we have better docs to cross link to)...

What do y'all think?

snewcomer · 2018-01-12T17:22:44Z

@rwjblue It looks like the tests are passing but for a focus-in test in ember-glimmer.

snewcomer commented Jan 7, 2018

View reviewed changes

snewcomer commented Jan 8, 2018

View reviewed changes

snewcomer force-pushed the no-jquery-testing branch from 7912b24 to a1ae8bc Compare January 8, 2018 15:46

rwjblue requested changes Jan 8, 2018

View reviewed changes

wycats mentioned this pull request Jan 8, 2018

[QUEST] Making jQuery Optional #16058

Closed

48 tasks

snewcomer force-pushed the no-jquery-testing branch from b57c2fe to da22320 Compare January 8, 2018 18:35

snewcomer commented Jan 8, 2018

View reviewed changes

rwjblue reviewed Jan 8, 2018

View reviewed changes

simonihmig reviewed Jan 8, 2018

View reviewed changes

snewcomer force-pushed the no-jquery-testing branch from 403df0f to 814ed21 Compare January 8, 2018 22:37

snewcomer commented Jan 8, 2018

View reviewed changes

snewcomer commented Jan 9, 2018

View reviewed changes

snewcomer force-pushed the no-jquery-testing branch from 9de52e9 to cf6ce62 Compare January 10, 2018 16:23

ro0gr reviewed Jan 11, 2018

View reviewed changes

snewcomer force-pushed the no-jquery-testing branch 3 times, most recently from 371ee49 to 59d6680 Compare January 12, 2018 05:17

remove jquery from ember-testing

3e005da

snewcomer force-pushed the no-jquery-testing branch from 59d6680 to 3e005da Compare January 12, 2018 15:34

rwjblue approved these changes Jan 14, 2018

View reviewed changes

rwjblue merged commit d7a4778 into emberjs:master Jan 14, 2018

snewcomer deleted the no-jquery-testing branch May 28, 2018 15:04


		setupForTesting();
		// [`@test #setupForTesting attaches ajax listeners`](assert) {

remove jquery from ember-testing #16075

remove jquery from ember-testing #16075

Conversation

snewcomer commented Jan 6, 2018 • edited Loading

ro0gr commented Jan 6, 2018

snewcomer commented Jan 6, 2018

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Jan 11, 2018

ro0gr Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Jan 12, 2018

snewcomer commented Jan 6, 2018 •

edited

Loading

ro0gr Jan 11, 2018 •

edited

Loading