feature(annotations): add two new types of annotations to optionally be more explicit about the type #96

iammerrick · 2015-02-01T00:04:18Z

This is to address minification issues by allowing folks to be more explicit.

iammerrick · 2015-02-01T00:06:12Z

src/util.js

@@ -1,17 +1,28 @@
-// A bunch of helper functions.
+import {


@vojtajina For some reason my import is consistently importing undefined. Am I doing something wrong?

…be more explicit about the type chore(annotations): remove unneccessary import fix(annotations): fix cyclic dependency

tusharmath · 2015-02-01T03:24:28Z

src/providers.js

+  else if (clsOrFunction.name) {
+    return isUpperCase(clsOrFunction.name.charAt(0));
+  } else {
+    return Object.keys(clsOrFunction.prototype).length > 0;


Can we test for prototype first and then function name?

Object.keys() will only return enumerable properties, and class methods are not going to be enumerable. Object.getOwnPropertyNames() would work better for those, but would exclude symbol methods, so not that great either.

After the annotations it is falling back to the existing functionality.

I'm not sure why you think that matters, it's going to be broken in the future

Just didn't seem relevant for this particular pull request, though I totally agree if you'd like to address that now.

we can't get rid of the fallbacks, but I think we can fix up the Object.keys thing to make sure we can still get at non-enumerable methods once classes ship in M42 or M43

@caitp would you like me to switch over to getOwnPropertyNames?

there are 2 things:

Reflect.ownKeys(O) (not implemented yet anywhere), but this would be the best

Combination of Object.getOwnPropertyNames(O) and Object.getOwnPropertySymbols(O) --- but you'd have to be careful with getOwnPropertySymbols(O) since it's not ES5.

Ok. So would you like me to try and address this using number 2 for now and then number 1 in the future?

P.S. You're really smart, I had no idea about this problem. :-)

… compliant

…s were caught

iammerrick · 2015-02-02T06:00:42Z

@caitp Anything else on this before it can be merged? :-)

tusharmath · 2015-02-02T06:08:56Z

src/util.js

+  if (Object.getOwnPropertySymbols) return keys.concat(Object.getOwnPropertySymbols(O));
+  return keys;
+});
+

 export {
  isUpperCase,
  isClass,


isClass has not been defined but being returned.

tbosch · 2015-02-02T23:54:53Z

Hi,
thanks for the PR. However, we are building a new version of DI within the angular/angular project right now, and won't continue the development of DI in this repository. And there we already differentiate between classes and factories.

iammerrick · 2015-02-02T23:56:51Z

Man that would have been great before I made like all those commits based on @caitp's feedback. :-( Will the other one be published to NPM under the same name? How different is it? Can it be used in Node too?

iammerrick · 2015-02-03T00:09:15Z

Hmmm. I'm thinking maybe I'll fork and maintain an ES6 specific version. I've got an entire application written in this, how different is the other framework? Will it require atscript for use? When can I start using the other framework? I'm effectively deadlocked on this issue till the other framework is released. I am feeling very frustrated that this wasn't communicated earlier? Was it already communicated or did I miss something?

iammerrick · 2015-02-03T00:15:09Z

I'm feeling very frustrated by this experience... :-(

caitp · 2015-02-03T00:24:36Z

I did tell you this wasn't being used by angular :) However, I don't think there's any reason why it couldn't be merged. It would just need an owner to ship it on npm

iammerrick · 2015-02-03T00:27:46Z

If you plan on publishing an entirely different project under the di on npm this repo should have a warning on it. I misunderstood what you meant when you said that @caitp. Not being used by angular and being deprecated and eventually replaced are different things.

iammerrick · 2015-02-03T00:29:41Z

And thank you @caitp for your help. I apologize to all for my raging. Just very frustrated by this decision.

caitp · 2015-02-03T00:29:46Z

it's not clear that it will be replaced by angular/angular's DI implementation (it's a possibility, but that would be the third totally separate DI library published with that name on npm, and that's getting a bit crazy and confusing =)

iammerrick · 2015-02-03T00:30:43Z

Oh thats good to know. :-) @tbosch's comment would make one believe otherwise.

iammerrick · 2015-02-03T00:32:37Z

I think under that looming possibility it makes sense for me to fork the project, remove the Traceur style annotations and maintain a separate repo. I hate the thought of that but it seems irresponsible for me to keep building a businesses product on such unstable ground. I had the impression this project was much more concrete than it is.

caitp · 2015-02-03T00:34:04Z

I suppose there's no reason you can't do that so long as the apache license terms are met

iammerrick · 2015-02-03T00:35:19Z

I think I'm going to rewrite a similar lib with vanilla es6.

tusharmath · 2015-02-04T03:21:45Z

Yay!

iammerrick · 2015-02-04T03:22:27Z

WAHOO! THANK YOU! @vsavkin

iammerrick · 2015-02-04T17:12:31Z

@vsavkin It seems the latest push to npm is missing the "dist" folder? Does a build need to be run before publishing?

xmlking · 2015-02-11T10:25:37Z

@iammerrick I am getting following error with this code change when transpiled to AMD. Can you change from this.Reflect to Reflect?

Error Cannot read property 'Reflect' of undefined

var ownKeys = (this.Reflect && Reflect.ownKeys ? Reflect.ownKeys : function OwnKeys(O) {
  var keys = Object.getOwnPropertyNames(O);
  if (Object.getOwnPropertySymbols) return keys.concat(Object.getOwnPropertySymbols(O));
  return keys;
});

angular#96

angular/di.js#96

iammerrick reviewed Feb 1, 2015
View reviewed changes

feature(annotations): add two new types of annotations to optionally …

d9d58f9

…be more explicit about the type chore(annotations): remove unneccessary import fix(annotations): fix cyclic dependency

iammerrick force-pushed the class-factory-annotations branch from 276cbce to d9d58f9 Compare February 1, 2015 00:21

tusharmath reviewed Feb 1, 2015
View reviewed changes

iammerrick added 4 commits February 1, 2015 13:08

feature(annotations): export new annotations

93efb35

fix(annotations): isClass annotation to leverage methods to be future…

72c7ca0

… compliant

fix(annotations): improved ownKeys implementation and moved it to util

ed1137c

fix(providers): added test to be sure classes with non-enumerable key…

7494392

…s were caught

tusharmath reviewed Feb 2, 2015
View reviewed changes

fix(util): remove leftover export

a52ee97

vsavkin merged commit a52ee97 into angular:master Feb 4, 2015

xmlking added a commit to xmlking/di.js that referenced this pull request Feb 11, 2015

this.Reflect -> Reflect

a8ee425

angular#96

xmlking added a commit to xmlking/spa-starter-kit that referenced this pull request Feb 11, 2015

Fix for Error: Cannot read property 'Reflect' of undefined

fd95db7

angular/di.js#96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(annotations): add two new types of annotations to optionally be more explicit about the type #96

feature(annotations): add two new types of annotations to optionally be more explicit about the type #96

iammerrick commented Feb 1, 2015

iammerrick Feb 1, 2015

tusharmath Feb 1, 2015

caitp Feb 1, 2015

iammerrick Feb 1, 2015

caitp Feb 1, 2015

iammerrick Feb 1, 2015

caitp Feb 1, 2015

iammerrick Feb 1, 2015

caitp Feb 1, 2015

iammerrick Feb 1, 2015

iammerrick Feb 1, 2015

iammerrick commented Feb 2, 2015

tusharmath Feb 2, 2015

tbosch commented Feb 2, 2015

iammerrick commented Feb 2, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

tusharmath commented Feb 4, 2015

iammerrick commented Feb 4, 2015

iammerrick commented Feb 4, 2015

xmlking commented Feb 11, 2015

feature(annotations): add two new types of annotations to optionally be more explicit about the type #96

feature(annotations): add two new types of annotations to optionally be more explicit about the type #96

Conversation

iammerrick commented Feb 1, 2015

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

iammerrick commented Feb 2, 2015

Choose a reason for hiding this comment

tbosch commented Feb 2, 2015

iammerrick commented Feb 2, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

iammerrick commented Feb 3, 2015

caitp commented Feb 3, 2015

iammerrick commented Feb 3, 2015

tusharmath commented Feb 4, 2015

iammerrick commented Feb 4, 2015

iammerrick commented Feb 4, 2015

xmlking commented Feb 11, 2015