Skip to content
This repository has been archived by the owner on Jan 12, 2020. It is now read-only.

Pass owner to constructor. #18

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions addon/component-managers/sparkles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getOwner, setOwner } from '@ember/application';
import ApplicationInstance from '@ember/application/instance';
import { capabilities } from '@ember/component';
import SparklesComponent from 'sparkles-component';
import { deprecate } from '@ember/debug';

export interface ComponentManagerArgs {
named: object;
Expand All @@ -16,17 +17,24 @@ export default class SparklesComponentManager {
return new this(owner);
}
capabilities: any;
constructor(owner: ApplicationInstance) {
setOwner(this, owner);
constructor(private owner: ApplicationInstance) {
this.capabilities = capabilities('3.4', {
destructor: true,
asyncLifecycleCallbacks: true,
});
}

createComponent(Klass: typeof SparklesComponent, args: ComponentManagerArgs): CreateComponentResult {
let instance = new Klass(args.named);
setOwner(instance, getOwner(this));
let { owner } = this;

let instance = new Klass(args.named, owner);

// check to make sure that owner was properly set, this ensures `super(...arguments)` was called
if (getOwner(instance) !== owner) {
deprecate(`must call super with all arguments in the constructor for ${Klass.name}`, false, { id: 'sparkles-component.super-arguments', until: '2.0.0' });
setOwner(instance, owner);
}

return instance as CreateComponentResult;
}

Expand Down
7 changes: 6 additions & 1 deletion addon/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { setComponentManager } from '@ember/component';
import { setOwner } from '@ember/application';

class SparklesComponent<T = object> {
constructor(public args: T) {}
constructor(public args: T, owner: any | undefined) {
Copy link

Choose a reason for hiding this comment

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

This is swapped now I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

should be

Suggested change
constructor(public args: T, owner: any | undefined) {
constructor(public args: T, owner?: {}) {

to describe the following constraints:
(a) undefined need not be passed explicitly under any circumstances
(b) if an argument is passed, it cannot be null or undefined

Copy link
Owner Author

Choose a reason for hiding this comment

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

@mike-north - Seems good, should I use object or {}? (mostly just curious to the reasoning)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwjblue

let x: object = '';  // ❌
let x: {} = '';  // ✅

If you want to allow string, number Symbol and boolean, use {}
If you just want things that are assignable to JS Objects, use object

if (owner) {
setOwner(this, owner);
}
}

didInsertElement() {}
didUpdate() {}
Expand Down
42 changes: 41 additions & 1 deletion tests/integration/components/sparkles-component-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { setupRenderingTest } from 'ember-qunit';
import { render, clearRender, click } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import { getOwner } from '@ember/application';
import Service from '@ember/service';

module('Integration | Component | sparkles-component', function(hooks) {
let InstrumentedComponent;
Expand Down Expand Up @@ -118,6 +119,26 @@ module('Integration | Component | sparkles-component', function(hooks) {
assert.dom('p').hasText('HELLO!');
});

test('it can access injections in constructor', async function(assert) {
this.owner.register('service:something', Service.extend({
someMethod() { return 'it works!'; }
}));
class ComponentUnderTest extends SparklesComponent {
constructor() {
super(...arguments);

let owner = getOwner(this);
let something = owner.lookup('service:something');
this.text = something.someMethod();
}
}
this.owner.register('component:under-test', ComponentUnderTest);
this.owner.register('template:components/under-test', hbs`<p>{{this.text}}</p>`);

await render(hbs`<UnderTest />`);
assert.dom('p').hasText('it works!');
});

test('it can use tracked to recompute when args change', async function(assert) {
this.owner.register('component:under-test', class extends SparklesComponent {
@tracked('args')
Expand Down Expand Up @@ -221,5 +242,24 @@ module('Integration | Component | sparkles-component', function(hooks) {
);
await render(hbs`<UnderTest />`);
assert.dom('p').hasText('Environment: test');
})
});

test('not calling super with all arguments issues deprecation', async function(assert) {
this.owner.register('component:under-test', class UnderTest extends SparklesComponent {
constructor(args) {
super(args);
}
get environment() {
return getOwner(this).resolveRegistration("config:environment").environment;
}
});
this.owner.register(
'template:components/under-test',
hbs`<p>Environment: {{this.environment}}</p>`
);
await render(hbs`<UnderTest />`);
assert.dom('p').hasText('Environment: test');

assert.deprecationsInclude(`must call super with all arguments in the constructor for UnderTest`);
});
});
4 changes: 2 additions & 2 deletions tests/integration/components/tracked-ts-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ module('tracked: ts', function(hooks) {
class UnderTest extends SparklesComponent {
_value: number;

constructor(arg: {}) {
super(arg);
constructor(args: {}, owner: any) {
super(args, owner);
this._value = 0;
}

Expand Down
45 changes: 45 additions & 0 deletions tests/test-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,53 @@ import Application from '../app';
import config from '../config/environment';
import { setApplication } from '@ember/test-helpers';
import { start } from 'ember-qunit';
import { registerDeprecationHandler } from '@ember/debug';
import QUnit from 'qunit';
import 'qunit-dom';

setApplication(Application.create(config.APP));

start();

let deprecations;
registerDeprecationHandler((message, options, next) => {
// in case a deprecation is issued before a test is started
if (!deprecations) {
deprecations = [];
}

deprecations.push(message);
next(message, options);
});

QUnit.testStart(function() {
deprecations = [];
});

QUnit.assert.noDeprecations = function(callback) {
let originalDeprecations = deprecations;
deprecations = [];

callback();
this.deepEqual(deprecations, [], 'Expected no deprecations during test.');

deprecations = originalDeprecations;
};

QUnit.assert.deprecations = function(callback, expectedDeprecations) {
let originalDeprecations = deprecations;
deprecations = [];

callback();
this.deepEqual(deprecations, expectedDeprecations, 'Expected deprecations during test.');

deprecations = originalDeprecations;
};

QUnit.assert.deprecationsInclude = function(expected) {
this.pushResult({
result: deprecations.indexOf(expected) > -1,
actual: deprecations,
message: `expected to find \`${expected}\` deprecation`,
});
};