Remove jQuery #45

jeffjewiss · 2017-09-26T13:55:56Z

Removes jQuery as a dependency.

Fixes #37

todo:

get Node 4 running the tests on Travis and Appveyor
cleanup .travis.yml
confirm if Ember 1.13 can work with these changes
confirm Percy snapshots are the same as master branch

fotinakis · 2017-09-26T15:27:27Z

addon/snapshot.js

@@ -1,6 +1,10 @@
 import Ember from 'ember';
 import { getNativeXhr } from './native-xhr';
 import { maybeDisableMockjax, maybeResetMockjax } from './mockjax-wrapper';
+import $ from 'jquery';


😄

If you're interested in updating ember-cli-babel to version 6.3+ (I believe) you could also use the new Ember imports outlined here: https://github.com/ember-cli/ember-rfc176-data but that would likely be appropriate for another PR. There are tools to automate the change as well. Let me know if you're interested!

- setup ember-try to only include the event dispatcher in compatible version of Ember - add a targets config - reformat testem config - configure the build to only remove jquery if the events dispatcher is available - fix integration component test

jeffjewiss · 2017-10-09T12:26:12Z

Hi @fotinakis, this seems to be in a working state, but could use some further testing to make sure the proper snapshots are being taken. Regarding your previous instructions, should I be able to npm link this repo in the project where I have Percy setup and then run my tests with the access token present and it should upload snapshots to Percy? Then I can check to see that the snapshots look okay?

Also, I needed to change 1 test as it seems like there are still some Ember internals that use jQuery related to handling checkbox input events. I also updated some dependencies and had the tests run Node 6, and 8 instead of 4. I upgraded jshint to eslint as there is a plugin that helps with checking that jQuery isn't used in the add-on by enabling a linting rule. I used a tool called polyjuice to generate the eslint config from the preexisting jshint ones. Otherwise, I tried not to change things unrelated to removing jQuery or dependencies that could be left as is.

Please let me know when you have a chance to review this and if there are any changes you'd like to see before it's acceptable to merge. Thanks!

fotinakis

@jeffjewiss this is looking good, thanks for all the work here! Added some review comments here.

As for the tests, yes you can npm link it into a project to test (I will plan on doing that for our own projects before merging this). Also easier: just create a new project like test-ember-percy in percy, checkout master of this addon itself and run the tests pointed at that project, then checkout your branch locally and run the tests again to make sure nothing has changed. You can also run it locally with PERCY_BRANCH=test npm test to override the that Percy thinks it's coming from, which will default to master.

Let me know if that all makes sense or any questions on my comments here.

fotinakis · 2017-10-11T21:24:59Z

addon/snapshot.js

 import { getNativeXhr } from './native-xhr';
 import { maybeDisableMockjax, maybeResetMockjax } from './mockjax-wrapper';
+import ajax from 'ember-fetch/ajax';
+import run from 'ember-runloop';


I'm kind of confused as to why this works, and can't find any references to doing it this way. Is there a preference for this over destructuring run off of the Ember object?

This is working because ember-cli-shims is included as a dependency. Destructuring from the Ember object is now generally not recommended.

Ember is slowly moving towards using separate packages, so the preference is to import from these packages so that when future improvements are added to the build process (tree-shaking with rollup) it will result in smaller build files.

import run from 'ember-runloop'; then becomes import { run } from '@ember/runloop'; and a list of these new module imports can be found here: https://github.com/ember-cli/ember-rfc176-data

The documentation and guides are also in the process of being updated as of [email protected] and you can see the beginning of this: https://emberjs.com/api/ember/2.16/classes/@ember%2Frunloop

Makes total sense, thanks for the great explanation! Looks like I've been slacking on keeping my Ember knowledge up to date. :)

You're welcome, glad that was helpful. No worries, it's a lot of work to stay up to date. 😄

fotinakis · 2017-10-11T21:27:06Z

.travis.yml


 install:
+  - mkdir travis-phantomjs


It seems in the testem config you updated it to not use phantomjs at all — which is 👍 , but I think this needs to be updated and cleaned up as well. You can probably just use Travis's normal method for making sure Chrome is available: https://docs.travis-ci.com/user/chrome

Okay cool, looks like this was left over as I updated the Travis config in stages. I'll clean up the .travis.yml file.

fotinakis · 2017-10-11T21:28:47Z

.travis.yml

-  - PERCY_ENABLE=0 EMBER_TRY_SCENARIO=ember-1.13
-  - PERCY_ENABLE=0 EMBER_TRY_SCENARIO=ember-lts-2.4
+  - PERCY_ENABLE=0 EMBER_TRY_SCENARIO=ember-lts-2.8
+  - PERCY_ENABLE=0 EMBER_TRY_SCENARIO=ember-lts-2.12


Will we have to break backwards-compatibility with Ember 1.13 as part of this change? If not, let's keep the ember-1.13 try here.

Overall, if it required to drop 1.13 support, we're probably ready for that and fine with it, just need to bump a major release of this after it is merged.

I'll look at getting Node 4 working on Travis first, and then try adding this back in.

These changes should work fine with Ember 1.13. I removed that version to use more recent versions of Ember and because there seemed to be some weird caching/installation behaviour with ember-try on Travis.

Sounds good. If it ends up being a lot of work to get ember-try with 1.13 tests to work, then feel free to drop.

fotinakis · 2017-10-11T21:30:14Z

.travis.yml

@@ -1,19 +1,25 @@
 ---
 language: node_js
 node_js:
-  - "4"
+  - 6


We'd like to maintain Node 4 compatibility, especially because there are Node 4 LTS releases that don't fall out of maintenance until April 2018 (https://github.com/nodejs/Release).

Can you add back Node 4 here, and see comment below in package.json about Node 4 version.

Yes, no problem. I'll add that back in.

I initially removed it because I was able to have the add-on build and pass all the tests locally running Node 4, however Travis was throwing errors. The error seemed to be related to a missing "use strict;", but I had added it to the referenced file. So I think there may be a caching or other issue.

fotinakis · 2017-10-11T21:31:08Z

tests/.eslintrc.js

+  env: {
+    browser: true
+  },
+  globals: {},


I'm not sure why you didn't have to have percySnapshot defined here, if eslint still passes then great.

I updated the tests to use import { percySnapshot } from 'ember-percy';. This imports from the add-on instead of having to use the test helper.

I could either keep this method and remove the test helper, or revert to using the helper and add percySnapshot to the eslint globals. I thought it would be better to not use globals. Which do you prefer?

Ah very interesting. So our current docs say to ignore it here and import our register-helpers module to install our Ember async helper, so that in acceptance tests you always rely on the global and it works well with the other async helpers like visit, click etc. This is required in normal acceptance tests so that it doesn't have to be nested in an andThen() to get into the right order.

Since you've migrated the tests here to ember-native-dom-helpers and are using await it may not matter anymore in this case, the only slightly weird part I can think of is that we like using this repository as an example of writing Percy tests the recommended way. I'm not sure of the state of grand testing unification, but we'll need to support the Ember async helpers workflow for a long while.

Just some thoughts, I don't really have strong feelings here either way — just wanted to point out the effect on our docs and our currently recommended workflow for acceptance tests. What do you think we should do here?

Thanks for the detailed explanation. I agree that consistency and being able to use this repo as an example of writing Percy tests is important. So with that in mind I added the percySnapshot global back in for tests. I think it makes sense to keep and support the async helper.

fotinakis · 2017-10-11T21:31:55Z

package.json

@@ -16,40 +16,46 @@
  },
  "repository": "https://github.com/percy/ember-percy",
  "engines": {
-    "node": ">= 0.12.0"
+    "node": "^4.5 || 6.* || >= 7.*"


Related to comment in travis.yml, we'd like to maintain backwards compatibility for Node 4.x releases if possible. Can you explain a bit more about this line change?

I pulled this engine setup from here: https://github.com/ember-cli/ember-cli/blob/master/package.json#L148

I've generally seen this mentioned as the recommendation by ember-cli and for ember projects/add-ons. I believe the idea is to support LTS versions of Node, but with the preference of a more stable version of 4.

jeffjewiss · 2017-10-13T16:39:07Z

@fotinakis thanks for taking a look at this, I realize it's a big change.

Thanks for all the comments. I've answered each one and then added todos at the top of the PR for any outstanding tasks that came out of the comments. I'll work through them and ping you again when they're complete.

* upgrade ember-cli to 2.16 * config travis and ember-cli to test Ember 1.13

fotinakis · 2017-10-13T17:06:34Z

@jeffjewiss thanks for all the hard work on this PR! Responded to a few comments above.

jeffjewiss · 2017-10-17T15:15:47Z

@fotinakis I had to update the ajax call as the fetch API isn't the same as jQuery.ajax. In the process I also removed the mocks since jQuery isn't present and this should use native XHR by default.

I think I have all the issues fixed, however I ran out of builds on my Percy free trial. Would it be possible to get that reset or would you be able to take over confirming that the snapshots from this branch are the same as master?

fotinakis · 2017-10-28T01:12:57Z

@jeffjewiss sorry for the delayed response here. Yes definitely, I've just extended your trial. We also are about to merge something in #47 that will have a tiny impact on this PR and need to be merged in.

* master: 1.2.20 Use afterTests to call finalize (percy#47) 1.2.19 Update percy-client dependency (percy#46)

jeffjewiss · 2017-10-29T21:23:00Z

@fotinakis no problem! Okay great. Thanks for the heads up, I've merged master into this branch and made the appropriate updates to finalizeBuildOnce.

My snapshots are now uploading correctly. This revealed an issue with cloning textareas, but I was able to fix that with the last commit. I'm now seeing 37 snapshots and no visual diffs on my Percy Test Project which is just snapshots from this branch/PR.

I think this should be ready for a final review. 🎉

jeffjewiss · 2017-11-06T14:08:40Z

Hey @fotinakis, just wanted to ping you on this and see if you had a chance to look at it. Cheers!

* master: 1.2.21 Upgrade pinned version of percy-client.

jeffjewiss · 2017-12-04T13:31:29Z

Hey, just wanted to check in on this and see if anyone had a chance to take a look at it. Thanks!

topaxi · 2018-01-09T11:32:32Z

Ember.js is actively making jQuery optional, this would be a great time to get this landed 👍
emberjs/ember.js#16058

* master: TypeScript ambient type information (percy#50) 1.2.22 Update `ember-cli-babel` to v6.10.0 (percy#49)

RobbieTheWagner · 2018-02-08T03:03:25Z

How's this going @jeffjewiss @fotinakis? This is one thing keeping me from using Percy right now, since I need to remove jQuery from my apps. Do you guys need any help?

jeffjewiss · 2018-02-08T13:59:19Z

Hey @rwwagner90, from what I remember this PR was working (tests passing, and uploading the correct snapshots) back in October when I posted the update. I didn't hear back re: feedback and haven't looked into why the most recent merge broke things.

RobbieTheWagner · 2018-02-08T14:27:15Z

@fotinakis what do we need to do to get this in?

fotinakis · 2018-03-20T20:48:37Z

Closing in preference for #58

Thanks for pushing us to get this done @jeffjewiss. We decided on an interesting path that allows us to keep all of our current jquery-dependent behaviors in tests, while still allowing people to strip jquery from their production Ember builds.

use ajax from ember-fetch

0eec008

fotinakis reviewed Sep 26, 2017

View reviewed changes

jeffjewiss added 14 commits October 7, 2017 13:34

remove jquery usage from snapshot

43ee94e

upgrade from jshint to eslint

7bdc03f

make eslint config easier to read

a951f0a

setup eslint with ember rules to warn for jquery

39f9297

install dependencies for running tests without jquery

5d54538

upgrade ember-cli-babel and peer deps

010a7d7

use babel polyfill in testing

adcd1ff

use native dom helpers in tests

5f241e1

remove remaining references to jshint

b35c6fe

Finish removing jquery in the addon

866f396

- setup ember-try to only include the event dispatcher in compatible version of Ember - add a targets config - reformat testem config - configure the build to only remove jquery if the events dispatcher is available - fix integration component test

add use strict for node 4

f848919

install bower globally on appveyor

b951438

update travis config with latest

ee24ec4

try switching quotes for build script

4425ec7

jeffjewiss force-pushed the remove-jquery branch from d5feca0 to 9ade1d1 Compare October 7, 2017 22:24

try not caching yarn with travis

46770e4

jeffjewiss force-pushed the remove-jquery branch from 9ade1d1 to 46770e4 Compare October 7, 2017 22:30

use node 6/8 on travis, update engines

3bcbcef

jeffjewiss force-pushed the remove-jquery branch from 14a1b75 to 3bcbcef Compare October 7, 2017 22:41

jeffjewiss changed the title ~~[WIP] Remove jQuery~~ Remove jQuery Oct 7, 2017

jeffjewiss force-pushed the remove-jquery branch from b19fad8 to a855ae8 Compare October 7, 2017 22:57

test ember 2.8, 2.12, not 1.13, 2.4

9dc3f5b

jeffjewiss force-pushed the remove-jquery branch from a855ae8 to 9dc3f5b Compare October 7, 2017 23:10

jeffjewiss added 2 commits October 8, 2017 12:03

fix ember try config and update ember cli

ab67d27

add ember try files to gitignore

cec400f

fotinakis reviewed Oct 11, 2017

View reviewed changes

Have Travis and Appveyor test Node 4

3a16b39

Test Ember 1.13 with ember-try

f1b813e

* upgrade ember-cli to 2.16 * config travis and ember-cli to test Ember 1.13

jeffjewiss force-pushed the remove-jquery branch from 28a2425 to f1b813e Compare October 13, 2017 16:51

jeffjewiss force-pushed the remove-jquery branch from 9987d8c to fe45f67 Compare October 17, 2017 12:36

Clean up travis config

ca05a92

jeffjewiss force-pushed the remove-jquery branch from fe45f67 to ca05a92 Compare October 17, 2017 12:51

jeffjewiss added 3 commits October 17, 2017 13:57

Add headless chrome config to testem

cbedc0f

Fix usage of snapshot async helper

e1bfed2

Fix ajax call and remove jQuery mocks

f0d276d

jeffjewiss force-pushed the remove-jquery branch from f8ac372 to f0d276d Compare October 17, 2017 14:24

jeffjewiss added 2 commits October 29, 2017 19:06

Merge branch 'master' into remove-jquery

0f64872

* master: 1.2.20 Use afterTests to call finalize (percy#47) 1.2.19 Update percy-client dependency (percy#46)

Update textarea text/values after dom copy

4904f93

Merge branch 'master' into remove-jquery

7a38190

* master: 1.2.21 Upgrade pinned version of percy-client.

Merge branch 'master' into remove-jquery

a4d4345

* master: TypeScript ambient type information (percy#50) 1.2.22 Update `ember-cli-babel` to v6.10.0 (percy#49)

jeffjewiss force-pushed the remove-jquery branch from 27554e0 to a4d4345 Compare January 10, 2018 11:21

fotinakis closed this Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove jQuery #45

Remove jQuery #45

jeffjewiss commented Sep 26, 2017 •

edited

Loading

fotinakis Sep 26, 2017

jeffjewiss Sep 26, 2017

jeffjewiss commented Oct 9, 2017

fotinakis left a comment

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

fotinakis Oct 13, 2017

jeffjewiss Oct 17, 2017

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

fotinakis Oct 13, 2017

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

fotinakis Oct 13, 2017

jeffjewiss Oct 17, 2017

fotinakis Oct 11, 2017

jeffjewiss Oct 13, 2017

jeffjewiss commented Oct 13, 2017

fotinakis commented Oct 13, 2017

jeffjewiss commented Oct 17, 2017

fotinakis commented Oct 28, 2017

jeffjewiss commented Oct 29, 2017

jeffjewiss commented Nov 6, 2017

jeffjewiss commented Dec 4, 2017

topaxi commented Jan 9, 2018

RobbieTheWagner commented Feb 8, 2018

jeffjewiss commented Feb 8, 2018

RobbieTheWagner commented Feb 8, 2018

fotinakis commented Mar 20, 2018

Remove jQuery #45

Remove jQuery #45

Conversation

jeffjewiss commented Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffjewiss commented Oct 9, 2017

fotinakis 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

jeffjewiss commented Oct 13, 2017

fotinakis commented Oct 13, 2017

jeffjewiss commented Oct 17, 2017

fotinakis commented Oct 28, 2017

jeffjewiss commented Oct 29, 2017

jeffjewiss commented Nov 6, 2017

jeffjewiss commented Dec 4, 2017

topaxi commented Jan 9, 2018

RobbieTheWagner commented Feb 8, 2018

jeffjewiss commented Feb 8, 2018

RobbieTheWagner commented Feb 8, 2018

fotinakis commented Mar 20, 2018

jeffjewiss commented Sep 26, 2017 •

edited

Loading