Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Using angular/di.js in a babel runtime gives error #103

Open
dmtrs opened this issue May 16, 2015 · 23 comments
Open

Using angular/di.js in a babel runtime gives error #103

dmtrs opened this issue May 16, 2015 · 23 comments

Comments

@dmtrs
Copy link
Contributor

dmtrs commented May 16, 2015

My setup is babel v5.4.3 and mocha testing suite and I am trying to use di ^2.0.0-pre-14.

I have a require hook for my tests that looks like:

require("babel/register")({
    stage: 0
});

I have created a a file names src/rest.js that looks like this (request-promise):

import {Inject} from 'di';
import {default as Request} from 'request-promise';

@Inject(Request)
export class Rest {
    constructor(request) {
        this.request = request;
    }
}

following my src/rest.test.js looks like this:

import { Injector } from "di";
import { Rest } from "./rest";
import { should } from "chai";
should();

describe('Rest', function(){
    var injector = new Injector([]);
    describe('#constructor()', function(){
        it('should x', function(){ })
    })
})

when running the test I get the following error:

./node_modules/mocha/bin/mocha --require babelhook.js src/rest.test.js

/Users/dmtrs/code/sc.io/node_modules/di/dist/cjs/annotations.js:12
  this.tokens = tokens;
              ^
TypeError: Cannot set property 'tokens' of undefined
    at Inject (/Users/dmtrs/code/sc.io/node_modules/di/dist/cjs/annotations.js:12:15)
    at /Users/dmtrs/code/sc.io/src/rest.js:1:15
    at Object.<anonymous> (/Users/dmtrs/code/sc.io/src/rest.js:5:18)
    at Module._compile (module.js:456:26)
    at normalLoader (/Users/dmtrs/code/sc.io/node_modules/babel/node_modules/babel-core/lib/babel/api/register/node.js:150:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/dmtrs/code/sc.io/node_modules/babel/node_modules/babel-core/lib/babel/api/register/node.js:163:7)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/Users/dmtrs/code/sc.io/src/rest.test.js:1:30)
    at Module._compile (module.js:456:26)
    at normalLoader (/Users/dmtrs/code/sc.io/node_modules/babel/node_modules/babel-core/lib/babel/api/register/node.js:150:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/dmtrs/code/sc.io/node_modules/babel/node_modules/babel-core/lib/babel/api/register/node.js:163:7)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at /Users/dmtrs/code/sc.io/node_modules/mocha/lib/mocha.js:192:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/dmtrs/code/sc.io/node_modules/mocha/lib/mocha.js:189:14)
    at Mocha.run (/Users/dmtrs/code/sc.io/node_modules/mocha/lib/mocha.js:422:31)
    at Object.<anonymous> (/Users/dmtrs/code/sc.io/node_modules/mocha/bin/_mocha:398:16)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:935:3
@caitp
Copy link
Contributor

caitp commented May 16, 2015

The latest commit fixes this, but it's not published on npm yet, one of the Googlers need to do that. In the mean time, you can point to the github repo in package.json

@caitp
Copy link
Contributor

caitp commented May 16, 2015

@vicb or @btford or someone might be willing to publish an update just for convenience

@dmtrs
Copy link
Contributor Author

dmtrs commented May 16, 2015

@caitp I wil have a look and close the issue. Thanks for the moment

@dmtrs
Copy link
Contributor Author

dmtrs commented May 17, 2015

Unfortunately the issue remains. What I did was to clone the repo with latest commit 9fd8c04d9d3182e5f19128af26ab84b66e56e633 used gulp to build the cjs folder as is and then moved everything under my project's node_module/di folder.

And the issue still remains.

@caitp
Copy link
Contributor

caitp commented May 17, 2015

I don't think so, post the contents of your compiled annotations file

@dmtrs
Copy link
Contributor Author

dmtrs commented May 17, 2015

@caitp
Copy link
Contributor

caitp commented May 17, 2015

I meant post it to like a gist or something =) Can you move that off the issue tracker please. Anyways, there may be more work needed to get it working with traceur. It may also need to be updated to match the current proposed decorator semantics.

@dmtrs
Copy link
Contributor Author

dmtrs commented May 17, 2015

@caitp I have removed the code and added to a gist.

You mean babel need update to much current proposed decorator semantics?

@caitp
Copy link
Contributor

caitp commented May 17, 2015

No, I mean di.js may need an update to match the current es7 proposal for decorators. Or, it may not. Angular isn't really using this library at the moment, so it's not clear what's needed until people file bugs

@dmtrs
Copy link
Contributor Author

dmtrs commented May 17, 2015

Could I overcome the issue if I use the following syntax?

import {annotate, Inject} from 'di';
import {default as Request} from 'request-promise';

export class Rest {
    constructor(request) {
        this.request = request;
    }
}
annotate(Rest, new Inject(Request));

and by this I mean if the behavior would be the same from part of the di.js lib

@vicb
Copy link

vicb commented May 17, 2015

I don't have the rights, may be @vsavkin ?

@L8D
Copy link

L8D commented May 18, 2015

This is because babel support decorators not annotations. See #104 for a fix.

@vsavkin
Copy link
Contributor

vsavkin commented May 19, 2015

I'll release a new version today.

@tamascsaba
Copy link

Hy, but the problem is same, and @L8D decorators fix is not the perfect solution (I think).

@L8D
Copy link

L8D commented May 29, 2015

Babel does not support annotations, it supports decorators. Annotations and decorators have the same syntax. The only way this library can seamlessly support both decorators and annotations is to stop using ES6 classes in their definitions (to allow the constructors to be called without 'new'), and then to add a check as to whether they are being called (as decorators) or initialized (as annotations) and then to return the appropriate values (the decorator function or the annotation instance).

Something along the lines of this:

function Inject(...tokens) {
  if (this instanceof Inject) { // being initialized as an annotation
    this.tokens = tokens;
    this.isPromise = false;
    this.isLazy = false;
  } else { // being called as a decorator
    return function(fn) {
      annotate(fn, new Inject(...tokens));
    };
  }
}

The only other options are:

  • Provide the annotate function as a decorator, which would be used as @annotate(new Inject(Foo, Bar))
  • Provide explicit decorator functions, which would be used as @inject(Foo, Bar)

@tamascsaba
Copy link

Thanks, the quick answer, i think it is not so hard implement it to di,js. What do you think @vsavkin which is the ideal solution?

@vsavkin
Copy link
Contributor

vsavkin commented Jun 9, 2015

In the long run DI.js should only support decorators. Angular 2, for instance, will drop support for annotations and will support only decorators very soon.

However, to make the transition easier, we can support both decorators and annotations for a short period of time.

To do that we need to do the following:

If anyone submits a PR with the fix, I'll merge it.

@L8D
Copy link

L8D commented Jun 9, 2015

@vsavkin why not the solution I proposed?

@vsavkin
Copy link
Contributor

vsavkin commented Jul 1, 2015

@L8D In general I prefer not to rely on this pattern. A metadata record and the act of attaching that metadata record to an entity are two different things to me. For instance, the metadata can/should be made immutable. But I don't feel super strongly about it.

But I am more concerned with how we store the metadata. The ES7 spec defines the metadata API that is supposed to be used for this purpose. Using our own custom metadata storage (the annotations property) seems like a smell to me.

@L8D
Copy link

L8D commented Jul 6, 2015

@vsavkin what about storing annotations in symbol properties?

@vsavkin
Copy link
Contributor

vsavkin commented Jul 6, 2015

@L8D definitely better than what we do today. Why don't you want to use the metadata API? Because of the polyfill?

@nmabhinandan
Copy link

Any progress?

@bugthesystem
Copy link

Any update on this?

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

No branches or pull requests

8 participants