Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@connectTo() throws error because store is not available in unit tests #56

Closed
dannyBies opened this issue Aug 9, 2018 · 10 comments
Closed

Comments

@dannyBies
Copy link
Contributor

I'm submitting a bug report

Library Version:
1.0.0

Browser:
Chrome 67

Current behavior:
Currently I'm converting a large application to make use of the store. When trying to convert our huge (non staged components) unit test set the @connectto decorator throws an error because the store is not available.

Expected/desired behavior:
When running the tests I expect the @connectto decorator to not throw an error.

What is the motivation / use case for changing the behavior?
I'm trying to convert a large amount of unit tests to make use of the aurelia-store. On our components we want to use @connectto to keep our components as clean as possible but when using the decorator the tests throw an error because the store is not available. We are currently subscribing to the state in the component bind/unbind and not using the @connectto as a workaround which works fine but we'd rather use the decorator.

The error is valid because we instantiate the component like an ordinary class new component() instead of using staged components. (Un)fortunately our test set is very large and it will take time to convert everything to staged components.

I was wondering if there is any way to keep the decorator and the non staged components. Maybe it's possible to return in this piece of code and log a warning that the store is not available?

@dannyBies
Copy link
Contributor Author

As far as I can see current functionality wouldn't be affected by my proposed code change.

@zewa666
Copy link
Member

zewa666 commented Aug 9, 2018

How would returning there help you? The actual work of the decorator wouldnt happen in that case. Could you show a sample test and how that would work with the proposed fix?

@dannyBies
Copy link
Contributor Author

I can't create a sample at this moment but it boils down to:


//Component
@connectTo()
export class  Component {

}

//Test

it('uses store but gives an error atm', () => {
    const component = new Component();
  });

My idea was to not let the decorator do anything in this case as I'm not making use of the aurelia lifecycle in this test.

I would expect @connectto() and

this.subscription = this.store.state.subscribe(
  (state) => this.state = state
);

to behave the same way. The first errors while the second succeeds with the above example test case

@dannyBies
Copy link
Contributor Author

Preferably I would change all tests to make use of staged components but for now I unfortunately don't have the time to rewrite all those tests.

Still I think running tests like that should be a supported scenario even if you don't use the aurelia lifecycle.

@zewa666
Copy link
Member

zewa666 commented Aug 9, 2018

perhaps there is a ... frankly weird, solution. The issue originally stems from the fact that the decorator is parsed and used while aurelias DI comes too late. So in order to make that work we simply need to delay the parsing of the class.

Let's look at this situation:

// file: tests-not-working.ts

import {inject} from 'aurelia-dependency-injection';
import {Store, connectTo} from 'aurelia-store';
import {IState} from 'models/state';

@inject(Store)
@connectTo()
export class TestsNotWorking {
  public state: IState;
  public store: Store<IState>;

  constructor(store: Store<IState>) {
    this.store = store;
  }
}

and this original test

import {Container} from 'aurelia-dependency-injection';
import { Store } from 'aurelia-store';

import {TestsNotWorking} from 'tests-not-working';
import {IState} from 'models/state';
import {initialState} from 'models/initialState';

describe('App', () => {
  let container: Container;
  let store: Store<IState>;
  let sut: TestsNotWorking;

  beforeEach(() => {
    container = new Container().makeGlobal();
    store = new Store<IState>(initialState);
    container.registerInstance(Store, store);
  });

  it('store defined', () => {
    sut = new TestsNotWorking(store);
  });
});

Here the instantiation of the sut would fail as you mentioned.

Now this here would work

import {Container} from 'aurelia-dependency-injection';
import { Store } from 'aurelia-store';

import {IState} from 'models/state';
import {initialState} from 'models/initialState';

// Starting with TS2.9 this is a type-only sideeffect free import
declare type SUT = import("../../src/tests-not-working").TestsNotWorking;

describe('App', () => {
  let container: Container;
  let store: Store<IState>;
  let sut: SUT;

  beforeEach(() => {
    container = new Container().makeGlobal();
    store = new Store<IState>(initialState);
    container.registerInstance(Store, store);
  });

  it('store defined', async () => {
    // this is an actual lazy import
    const ctor = (await import('../../src/tests-not-working')).TestsNotWorking;
    sut = new ctor(store);

    console.log(ctor.prototype);
    // logs:
    // TestsNotWorking{activate: function () { ... }, bind: function () { ... }, unbind: function () { ... }}
  });
});

It does actually lazy-import the class and thus makes sure the store is properly present in DI before requested by the decorator. I'm just not sure if this has any side-effects. Might wanna try that out on your codebase? It could also potentially go wrong with some bundlers. Tested this with SystemJS

@dannyBies
Copy link
Contributor Author

dannyBies commented Aug 9, 2018

It looks a bit messy and hard to understand. I had hoped there would be a cleaner solution for this. Nevertheless I will try it out on monday. Thanks a lot for the help, it's much appreciated.

I'm using Webpack 3.x

@zewa666
Copy link
Member

zewa666 commented Aug 9, 2018

I agree its not very nice but maybe a way for now until you can migrate to staged components.

@dannyBies
Copy link
Contributor Author

Unfortunately I couldn't get it working on my current setup using the lazy import. I don't have enough time available to debug why it's not working so for now I will keep subscribing to the state in the bind() and slowly convert our tests to staged components so I can use the decorator.

@stevies
Copy link

stevies commented Aug 29, 2018

Any clue on how to get tests working for normal JavaScript, not Typescript?

import {connectTo, Store} from 'aurelia-store';
import {inject} from 'aurelia-framework';

@inject(Store)
@connectTo()
export class App {
  constructor(store) {
    this.message = 'Hello World';
    this.store = store;
  }
}

Test

import {Store} from 'aurelia-store';
import {Container} from 'aurelia-dependency-injection';
import {state} from '../../src/state';

describe('the app', () => {
  let container;
  let store;
  let sut;

  beforeEach(() => {
    container = new Container().makeGlobal();
    store = new Store(state);
    container.registerInstance(Store, store);
  });

  it('says hello', async () => {
    const Ctor = (await import('../../src/app')).App;
    sut = new Ctor(store);
    console.log(sut);
  });
});

Error:

	Failed: undefined is not an object (evaluating '__webpack_require__.t.bind')
		at _callee$ test/unit/app.spec.js:17
		at tryCatch node_modules/babel-polyfill/dist/polyfill.js:6900
		at invoke node_modules/babel-polyfill/dist/polyfill.js:7138
		at node_modules/babel-polyfill/dist/polyfill.js:6952
		at Promise node_modules/babel-polyfill/dist/polyfill.js:4400

@stevies
Copy link

stevies commented Aug 29, 2018

Might have answered my own question:

    const Ctor = (await require('../../src/app')).App;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants