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

Invalid assumption of a class to be a factory #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tusharmath
Copy link

Overview
Uglify converts all class names to single letter lower case ones the factory provider automatically get created instead of the class provider. This should help rectify it.

Reproducable: always
Browsers: Chrome 39.0.2171.95
Operating System: OS X
Steps to reproduce

  • Create two classes inside a closure
(function () {
    var di = require('di');

    function Apples(b) {
        this.bapples = b;
    }

    Apples.prototype.print = function () {
        console.log(this.bapples.getMyName());
    };

    function Bapples() {}
    Bapples.prototype.getMyName = function () {
        return 'My Name is TUshar';
    };

    di.annotate(Apples, new di.Inject(Bapples));

    injector = new di.Injector();
    a = injector.get(Apples);
    a.print();
})();
  • run the code - works perfectly fine
  • minify the source
! function () {
    var e = require("di"),
        Injector = e.Injector;

    function t() {
        // this.bapples = t
    }
    injector = new Injector();
    a = injector.get(t);
    a.print();
}();
  • run the code (minified version), throws the following error -
    a.print();
      ^
TypeError: Cannot call method 'print' of undefined
    at /Users/tusharmathur/Documents/Projects/Executables/JavaScript.min.js:11:4
    at Object.<anonymous> (/Users/tusharmathur/Documents/Projects/Executables/JavaScript.min.js:12:2)
    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:929:3

Review on Reviewable

@tusharmath
Copy link
Author

@vojtajina Not sure why the build is failing. I see that it doesn't even work in master. Would it be possible to merge without the build passing? This issue is not letting my patch go to production :(

After minifying your javascript it is possible that the class names get changed, such that they are no more adhering to the standard of starting a class name with a capital letter. Thus the isClass util fun returns false in such cases, even though the class has some fields set in the prototype object.

The solution is to first check the prototype object for keys and then check for class name.

Fixes angular#92
@tusharmath tusharmath changed the title Making it work with uglifyjs #92 Invalid assumption of a class to be a factory Dec 27, 2014
@iammerrick
Copy link
Contributor

Indeed seeing this issue.

@tusharmath
Copy link
Author

@vojtajina Can you please comment if this can be merged?

@iammerrick
Copy link
Contributor

I don't know that having prototype methods means something is a class. I think we need to be more explicit.

@tusharmath
Copy link
Author

I totally agree. But this logic is already being used to differentiate between classes and function . This pull request only refines the hack for the time being so that people who are using it in production don't face issues

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

Successfully merging this pull request may close these issues.

2 participants