Upgrade to ember cli 3.4 via ember-cli-update #251

Gaurav0 · 2018-10-12T17:01:50Z

Update: will rebase once #254 , #255 , and #256 are merged.

Update: rebased.

Turbo87 · 2018-10-13T12:06:44Z

@Gaurav0 what are the breaking changes that this contains?

Gaurav0 · 2018-10-13T18:16:30Z

@Turbo87 The only possible breaking change I can see is the drop in support for node 4 via ember-cli, but since that is a devDependency it is quite likely that isn't a breaking change. Once we update broccoli-lint-eslint, that part will break anyway.

Turbo87 · 2018-10-13T18:22:26Z

package.json

    "testdouble": "^3.1.0"
  },
  "engines": {
-    "node": ">= 4.0"
+    "node": "6.* || 8.* || >= 10.*"


this is also a breaking change

It looks like the same breaking change to me.

maybe, but breaking changes should be deliberate, not side effects of other changes that dont even mention the breaking change

Ok, will break out.

Turbo87 · 2018-10-13T18:22:51Z

package.json

+    "ember-cli-inject-live-reload": "^1.8.2",
+    "ember-cli-qunit": "^4.3.2",
+    "ember-cli-sri": "^2.1.1",
+    "ember-cli-template-lint": "^1.0.0-beta.1",


do we need that? we don't have any templates

It's in the new add-on blueprint for ember-cli-update 3.4

that does not answer my question. the thing we're building here is a build time addon without any templates. I don't see a reason why we would need this extra dependency if we're not going to use it anyway

Turbo87 · 2018-10-13T18:23:14Z

package.json

@@ -41,33 +44,40 @@
    "walk-sync": "^0.3.0"
  },
  "devDependencies": {
-    "broccoli-asset-rev": "^2.4.5",
+    "@ember/optional-features": "^0.6.3",


do we need that?

It's in the new add-on blueprint for ember-cli-update 3.4

Turbo87 · 2018-10-13T18:23:42Z

testem.js

-    "PhantomJS"
-  ],
-  "launch_in_dev": [
-    "PhantomJS",


this looks like a major change to me that IMHO deserves it's own dedicated PR

If you say so. Again, the new add-on blueprint makes this change as well.

Turbo87 · 2018-10-13T18:24:26Z

tests/dummy/config/targets.js


 module.exports = {
-  browsers: [
-    'ie 9',


removing IE9 support from the test suite is a breaking change

Ok, if you say so. ember-cli-update blueprint change again.

Gaurav0 · 2018-10-14T02:39:10Z

All of the changes you mentioned as breaking were performed automatically as part of ember-cli-update. Please let me know how you would prefer to proceed.

Gaurav0 · 2018-10-15T18:13:12Z

Update: will rebase once #254 , #255 , and #256 are merged.

Turbo87 · 2018-10-16T12:24:18Z

.travis.yml

+        - yarn test
+
+cache:
+  yarn: true


this is a duplicate

Turbo87 · 2018-10-16T12:24:28Z

.travis.yml

+      name: "Tests"
+      script:
+        - yarn lint:js
+        - yarn test


what does this do? why the extra stage?

So the ember-cli-update transformed things to use ember-try. I tried to revert all those changes and must have missed this. Thanks!

Turbo87 · 2018-10-16T12:25:01Z

README.md

@@ -108,7 +107,6 @@ module.exports = function(defaults) {

 </details>

-


No reason. This probably happened during the automatic update or the merge or the rebasing. It just removes an extra line anyway.

the sections in the README were intentionally divided by two blank lines to give them some padding

ok I'll add it back.

Turbo87 · 2018-10-16T12:26:24Z

lib/displayutils.js

+  return string;
+}
+
+exports.resultString = resultString;


what is this new code? this seems unrelated to the Ember CLI update 🤔

So the new code is a copy of tap-reporter.js and its dependencies from testem.js (version 1.x) so that I didn't have to get the tests to match the latest version (which now includes time it takes a test to run).

I can move it to a vendor directory if you would prefer.

I'd prefer to not fork any reporters. we should instead probably update the assertions here to use regex or something like that 🤔

I'd prefer not to either. Is there a chai assertion library that takes an array and checks if any of its elements matches (and does not match) a regex?

Found one. chai-things

Gaurav0 · 2018-10-16T14:22:07Z

@Turbo87 Thanks again for the timely review. I appreciate all the feedback.

Gaurav0 · 2018-10-16T15:54:41Z

@Turbo87 @rwjblue Can you rereview?

node-tests/.eslintrc.js

node-tests/test.js

package.json

.eslintrc.js

Turbo87

@rwjblue I think this is good to go. Can you do a final sanity check?

Gaurav0 · 2018-10-23T11:45:03Z

@rwjblue Can you please look at this?

Turbo87 added the breaking label Oct 13, 2018

Turbo87 reviewed Oct 13, 2018

View reviewed changes

Gaurav0 force-pushed the upgrade_to_ember_cli_3_4 branch 2 times, most recently from cb87057 to 07be9b5 Compare October 15, 2018 21:14

Turbo87 reviewed Oct 16, 2018

View reviewed changes

Gaurav0 force-pushed the upgrade_to_ember_cli_3_4 branch 2 times, most recently from f21e191 to f805a3f Compare October 16, 2018 14:58

Gaurav0 force-pushed the upgrade_to_ember_cli_3_4 branch from 20d0fa4 to 8b3bb98 Compare October 17, 2018 13:13

Turbo87 suggested changes Oct 17, 2018

View reviewed changes

node-tests/.eslintrc.js Outdated Show resolved Hide resolved

node-tests/test.js Outdated Show resolved Hide resolved

node-tests/test.js Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Gaurav0 added 14 commits October 17, 2018 10:34

Upgrade to ember-cli 3.4 via ember-cli-update

ecf9b48

run codemods

b037c0b

Fix README.md merge

85be2fc

fix eslint errors

45008a3

fix travis.yml merge error

bf3daff

cleanup

f303d25

ensure tests are run

56d8341

remove unnecessary addons

404b7ce

Get tests passing again

a4d45ac

fix for travis.yml comments

f165b1c

fix linting errors

5a6d217

Do not fork tap reporter / match regexes

3948b78

address comments

1810da2

remove chai-things

293abba

Gaurav0 force-pushed the upgrade_to_ember_cli_3_4 branch from aeb27c3 to 293abba Compare October 17, 2018 14:35

Turbo87 suggested changes Oct 17, 2018

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

fix typo

7b01707

Turbo87 approved these changes Oct 17, 2018

View reviewed changes

rwjblue approved these changes Oct 23, 2018

View reviewed changes

rwjblue merged commit 92d4394 into ember-cli:master Oct 23, 2018

Gaurav0 deleted the upgrade_to_ember_cli_3_4 branch October 23, 2018 13:31

		@@ -108,7 +107,6 @@ module.exports = function(defaults) {

		</details>

Upgrade to ember cli 3.4 via ember-cli-update #251

Upgrade to ember cli 3.4 via ember-cli-update #251

Conversation

Gaurav0 commented Oct 12, 2018 • edited Loading

Turbo87 commented Oct 13, 2018

Gaurav0 commented Oct 13, 2018

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

Gaurav0 commented Oct 14, 2018 • edited Loading

Gaurav0 commented Oct 15, 2018

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

Gaurav0 Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gaurav0 Oct 16, 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

Choose a reason for hiding this comment

Gaurav0 commented Oct 16, 2018

Gaurav0 commented Oct 16, 2018

Turbo87 left a comment

Choose a reason for hiding this comment

Gaurav0 commented Oct 23, 2018

Gaurav0 commented Oct 12, 2018 •

edited

Loading

Gaurav0 commented Oct 14, 2018 •

edited

Loading

Gaurav0 Oct 16, 2018 •

edited

Loading

Gaurav0 Oct 16, 2018 •

edited

Loading