Replace "klassy" with ES6 classes #183

Turbo87 · 2016-11-22T13:17:45Z

/cc @trentmwillis @rwjblue @dgeb @stefanpenner

…egration()

rwjblue · 2016-11-22T13:53:40Z

Will take some time to do a more thorough review, but I definitely approve the effort (and from prior conversations with @dgeb I believe he does as well).

Turbo87 · 2016-11-22T13:54:54Z

💚 ... thanks Travis :)

dgeb · 2016-11-22T14:06:48Z

Yeah, Klassy is a lib I will gladly retire. Thanks @Turbo87.

rwjblue · 2016-11-25T14:50:28Z

lib/ember-test-helpers/test-module-for-acceptance.js

  setupContext() {
-    this._super({ application: this.createApplication() });
-  },
+    super({ application: this.createApplication() });


This should be super.setupContext (bare super is only valid in constructor).

rwjblue · 2016-11-25T14:50:43Z

lib/ember-test-helpers/test-module-for-acceptance.js


  teardownContext() {
    Ember.run(() => {
      getContext().application.destroy();
    });

-    this._super();
-  },
+    super();


Should be super.teardownContext

rwjblue · 2016-11-25T14:51:23Z

lib/ember-test-helpers/test-module-for-component.js

-  setupContext: function() {
-    this._super.call(this);
+  setupContext() {
+    super();


bare super is only valid in constructor

rwjblue · 2016-11-25T14:51:43Z

lib/ember-test-helpers/test-module-for-integration.js

@@ -93,7 +95,7 @@ export default AbstractTestModule.extend({
      return container.lookupFactory(subjectName);
    };

-    this._super({
+    super({


bare super is only valid in constructor

rwjblue · 2016-11-25T14:51:58Z

lib/ember-test-helpers/test-module.js

    var subjectName = this.subjectName;
    var container = this.container;

    var factory = function() {
      return container.lookupFactory(subjectName);
    };

-    this._super({
+    super({


bare super is only valid in constructor

ouch, forgot about that... thanks!

Turbo87 · 2016-11-25T16:32:06Z

@rwjblue done!

rwjblue

We will need to update ember-cli-qunit / ember-qunit and ember-cli-mocha / ember-mocha to prevent them from erroring (which I think makes this a major version bump, but that seems fine to me).

i.e.:

In the long run, I suspect that we should probably rethink our publishing to make it easier to avoid this sort of silly coupling but that seems fine for a future refactor to me...

Turbo87 · 2016-11-26T14:27:39Z

@rwjblue I do agree that those need updates to remove klassy, but I don't see why they would error? The way I see it we would just include an unnecessary dependency if we didn't update them, but everything should still be running the same way as before.

Turbo87 · 2016-11-26T15:30:45Z

also I tend to say that klassy is an implementation detail and as such private API so no need for a major version bump, but we obviously will have to test whether the new ember-test-helpers version would run smoothly with older testing libs like ember-cli-qunit.

rwjblue · 2016-11-26T17:11:49Z

I don't see why they would error?

When this PR lands the klassy dep will no longer be present, those calls to resolve.sync('klassy', { baseDir: emberTestHelpersPath }) will begin to error out.

rwjblue · 2016-11-26T17:15:56Z

I tend to say that klassy is an implementation detail and as such private API

I agree in general.

no need for a major version bump

Disagree, for the reason mentioned above. I believe that existing versions of ember-qunit / ember-cli-qunit will error after a rm -rf node_modules && npm install.

Turbo87 · 2016-11-26T18:42:55Z

I believe that existing versions of ember-qunit / ember-cli-qunit will error after a rm -rf node_modules && npm install.

I just tried it on a fresh app with npm link ember-test-helpers and both testing libs and didn't see any errors

rwjblue · 2016-11-26T23:45:16Z

I don't this npm link is good enough. You would need to ensure there is no version of klassy present in node_modules (which would not be the case if you installed normally then npm linked).

Turbo87 · 2016-11-27T09:42:00Z

You would need to ensure there is no version of klassy present in node_modules (which would not be the case if you installed normally then npm linked).

you're right, I'm stupid... I see how this would break now. Since klassy was a dependency of ember-test-helpers and not of ember-cli-qunit the resolver call would fail. Since we're sub-1.0 this will only require a minor version bump though to comply with semver.

rwjblue · 2016-11-27T13:39:11Z

Since we're sub-1.0 this will only require a minor version bump though to comply with semver.

Ya. I was thinking 0.6.0 (I sometimes incorrectly refer to this as a "major" bump when sub 1.0, but what I'm really meaning is that we need to not match for users with ^0.5.x).

Turbo87 · 2016-11-27T14:28:01Z

🎉

will adjust the other libs to account for this change later today unless someone else beats me to it 😉

Turbo87 added 12 commits November 22, 2016 14:29

Use ES6 shorthand methods

692d969

test-module-for-model: Inline "store" variable

1c7fecc

Simplify this._super() calls

8a724dc

test-module-for-model: Call this._super() before using "this"

1d00406

Extract "isPreGlimmer" constants

76ed205

Simplify setupComponentIntegrationTest() implementations

06843cc

test-module: Replace assignments with this._super() call

9450472

test-module-for-component: Simplify this._super() call

0e94056

test-module-for-component: Replace isComponentTestModule with initInt…

742d17e

…egration()

test-module-for-component: Extract integrationOption variable

450c43c

test-module-for-component: Call this._super() before using "this"

aac4b71

Replace "klassy" with ES6 classes

f3d9565

Turbo87 force-pushed the classes branch from 7afaa8e to f3d9565 Compare November 22, 2016 13:29

Turbo87 added the internal label Nov 22, 2016

rwjblue requested changes Nov 25, 2016

View reviewed changes

Use "super" properly

f6b9615

rwjblue reviewed Nov 26, 2016

View reviewed changes

rwjblue approved these changes Nov 27, 2016

View reviewed changes

rwjblue merged commit da6fa99 into emberjs:master Nov 27, 2016

Turbo87 deleted the classes branch November 27, 2016 14:27

This was referenced Nov 28, 2016

Update "ember-test-helpers" to v0.6.0-beta.1 emberjs/ember-qunit#249

Merged

Update "ember-test-helpers" to v0.6.0-beta.1 emberjs/ember-mocha#114

Merged

Update "ember-qunit" to v2.0.0-beta.1 ember-cli/ember-cli-qunit#157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace "klassy" with ES6 classes #183

Replace "klassy" with ES6 classes #183

Turbo87 commented Nov 22, 2016 •

edited

Loading

rwjblue commented Nov 22, 2016

Turbo87 commented Nov 22, 2016

dgeb commented Nov 22, 2016

rwjblue Nov 25, 2016

rwjblue Nov 25, 2016

rwjblue Nov 25, 2016

rwjblue Nov 25, 2016

rwjblue Nov 25, 2016

Turbo87 Nov 25, 2016

Turbo87 commented Nov 25, 2016

rwjblue left a comment

Turbo87 commented Nov 26, 2016

Turbo87 commented Nov 26, 2016

rwjblue commented Nov 26, 2016

rwjblue commented Nov 26, 2016

Turbo87 commented Nov 26, 2016

rwjblue commented Nov 26, 2016

Turbo87 commented Nov 27, 2016

rwjblue commented Nov 27, 2016

Turbo87 commented Nov 27, 2016

Replace "klassy" with ES6 classes #183

Replace "klassy" with ES6 classes #183

Conversation

Turbo87 commented Nov 22, 2016 • edited Loading

rwjblue commented Nov 22, 2016

Turbo87 commented Nov 22, 2016

dgeb commented Nov 22, 2016

rwjblue Nov 25, 2016

Choose a reason for hiding this comment

rwjblue Nov 25, 2016

Choose a reason for hiding this comment

rwjblue Nov 25, 2016

Choose a reason for hiding this comment

rwjblue Nov 25, 2016

Choose a reason for hiding this comment

rwjblue Nov 25, 2016

Choose a reason for hiding this comment

Turbo87 Nov 25, 2016

Choose a reason for hiding this comment

Turbo87 commented Nov 25, 2016

rwjblue left a comment

Choose a reason for hiding this comment

Turbo87 commented Nov 26, 2016

Turbo87 commented Nov 26, 2016

rwjblue commented Nov 26, 2016

rwjblue commented Nov 26, 2016

Turbo87 commented Nov 26, 2016

rwjblue commented Nov 26, 2016

Turbo87 commented Nov 27, 2016

rwjblue commented Nov 27, 2016

Turbo87 commented Nov 27, 2016

Turbo87 commented Nov 22, 2016 •

edited

Loading