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

minification issues #95

Open
markuz-brasil opened this issue Dec 28, 2014 · 13 comments
Open

minification issues #95

markuz-brasil opened this issue Dec 28, 2014 · 13 comments

Comments

@markuz-brasil
Copy link

First of all, I'm using 6to5 instead of traceur and mocha/chai/sinon instead of karma/jasmine. (and yeah, I ported the tests)

Flame war apart, the tests shouldn't fail. Which doesn't, unless it is minified. (And I didn't bother trying with minified traceur output...)

I pinpointed the issue to the class' context being somehow set to undefined in the minified form. So things like this.list fails because this is undefined.

Here is my project's git (minified) and all passing tests

I'm not clear where the issue actually is. I'm 99% certain that it isn't the testing framework. #93 doesn't fix it either. I'm left with this babel/babel#343, a bug on uglify or on the di framework itself. (or some combination of them)

My work around is something like this:
instead of writing this:

class UserList {}

class UserController {
  constructor(list) {
    this.list = list
  }
}
annotate(UserController, new Inject(UserList))

write this:

class UserList {}

class UserController {
  constructor(list) {
    this.list = list
  }
}

// add a factory around a class :/
annotate(UserFactory, new Inject(UserList))
function UserFactory (list) {
  return new UserController(list)
}

not ideal, but at least it gets the job done

@tusharmath
Copy link

@markuz-brasil How do you know this is not getting fixed with #93

@markuz-brasil
Copy link
Author

I took a look at the code, copied and pasted.
But I'm 100% convinced that the isClass and uglify are at fault.

I found out that uglifyy doesn't support Function.name link

I think that there is no way to distinguish a class from a function in ES5
(maybe ES6 too). So class Foo {} and function Foo(){} are exactly the
same.

I feel like the solution is with annotations. Maybe a @Factory and
isClasscheck for that with hasAnnotatiom(FnOrClass, Factory)

On Thursday, January 1, 2015, Tushar Mathur [email protected]
wrote:

@markuz-brasil https://github.com/markuz-brasil How do you know this is
not getting fixed with #93 #93


Reply to this email directly or view it on GitHub
#95 (comment).

@caitp
Copy link
Contributor

caitp commented Jan 1, 2015

I think that there is no way to distinguish a class from a function in ES5
(maybe ES6 too).

No, in ES6 they are deserialized as a class declaration or class expression (class BindingIdentifier ClassTail or class ClassTail)

@markuz-brasil
Copy link
Author

Cool. Good to know. Thanks

On Thursday, January 1, 2015, Caitlin Potter [email protected]
wrote:

I think that there is no way to distinguish a class from a function in ES5
(maybe ES6 too).

No, in ES6 they are deserialized as a class declaration or class expression


Reply to this email directly or view it on GitHub
#95 (comment).

@tusharmath
Copy link

@markuz-brasil Actually #93 is a workaround so that the class doesn't throw up errors.
In most real world scenarios every class would have some method. So if you update your code like this —

class UserController {
  constructor(list) {
    this.list = list
  }

  //Add a method to the prototype chain
  getFriends(){
    //In most cases you will always have a method.
  }
}

It should work.

@iammerrick
Copy link
Contributor

Indeed, here is an example project: https://github.com/iammerrick/webpack-jsx-uglify-bug

@vojtajina
Copy link
Contributor

There is no way to distinguish a class from a function, di.js uses the function/class name - if it starts with an uppercase letter, it's treated as a class. Sounds like the minifier changes class names into lowercase letters.

We can't treat everything as a class (call it with new operator), because if a non-class function returns a primitive value, this value wouldn't be used (the new object is used instead) and that's incorrect behavior.

We can add additional annotation such as @ClassProvider or @FactoryProvider.

Any other ideas?

@iammerrick
Copy link
Contributor

I think the additional annotation is our best option.

@markuz-brasil
Copy link
Author

I also agree.

Maybe something like @constructor as a hint for using ClassProvider and
the FactoryProvider as default way.

On Wednesday, January 7, 2015, Merrick Christensen [email protected]
wrote:

I think the additional annotation is our best option.


Reply to this email directly or view it on GitHub
#95 (comment).

@tusharmath
Copy link

Its just getting too verbose. Why can't we follow a convention as opposed to configuration?

Like any function with a name can be considered as a class otherwise its a factory.

@iammerrick
Copy link
Contributor

This is also a problem with @InjectLazy.

@tusharmath
Copy link

Is there a drawback of using the annotation approach?

@iammerrick
Copy link
Contributor

Addressing this with annotations here: https://github.com/angular/di.js/pull/96/files

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

5 participants