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

make module factory return a module name instead a module instance as… #232

Closed
wants to merge 1 commit into from

Conversation

drpicox
Copy link

@drpicox drpicox commented Oct 23, 2015

… angular libraries does

See: https://github.com/angular/bower-angular-animate/blob/fc8c108cbb1d8486f725386b943d35730caf3358/index.js as example:

    require('./angular-animate');
    module.exports = 'ngAnimate';

So you can do:

    module.exports = angular.module('yourModule', [
        require('./yourStuff'),
        require('./moreOfYourStuff'),
        require('angular-animate'),
        require('angular-route'),
        require('angular-chart'),
    ]).name;

… all angular libraries does

See: https://github.com/angular/bower-angular-animate/blob/fc8c108cbb1d8486f725386b943d35730caf3358/index.js as example:

    require('./angular-animate');
    module.exports = 'ngAnimate';

So you can do:

    module.exports = angular.module('yourModule', [
        require('./yourStuff'),
        require('./moreOfYourStuff'),
        require('angular-animate'),
        require('angular-route'),
        require('angular-chart'),
    ]).name;
@jtblin
Copy link
Owner

jtblin commented Nov 10, 2015

Didn't know about that. Is that backward compatible?

@drpicox
Copy link
Author

drpicox commented Nov 10, 2015

Nope, it is not backward compatible, at least in theory.

Why it is not backward compatible?

It is because users are doing:

module.exports = angular.module('yourModule', [
    require('./yourStuff'),
    require('./moreOfYourStuff'),
    require('angular-animate'),
    require('angular-route'),
    require('angular-chart').name,
]).name;

or

require('angular-chart');

module.exports = angular.module('yourModule', [
    require('./yourStuff'),
    require('./moreOfYourStuff'),
    require('angular-animate'),
    require('angular-route'),
    "chart.js",
]).name;

With the proposed change people can do:

module.exports = angular.module('yourModule', [
    require('./yourStuff'),
    require('./moreOfYourStuff'),
    require('angular-animate'),
    require('angular-route'),
    require('angular-chart'),
]).name;

or, not recommended - still the same -,

require('angular-chart');

module.exports = angular.module('yourModule', [
    require('./yourStuff'),
    require('./moreOfYourStuff'),
    require('angular-animate'),
    require('angular-route'),
    "chart.js",
]).name;

So, in the worst case, user must remove 5 characters.

@jtblin
Copy link
Owner

jtblin commented Nov 10, 2015

Ok, thanks. I am not going to introduce a breaking change right now. I'm waiting on chart.js 2.0 final release at which point there will be breaking changes so I'll publish a 1.0 for this library and can review this then.

@jtblin jtblin force-pushed the master branch 5 times, most recently from a871ce6 to 35d696f Compare April 16, 2016 09:47
jtblin added a commit that referenced this pull request Jun 19, 2016
Allow doing something like this:

```
var angular = require('angular');
var app = angular.module('app', [
  require('angular-chart')
]);
```
@jtblin
Copy link
Owner

jtblin commented Jun 19, 2016

Thanks for the idea, this is now done on chartjs-2.0 branch and will be published soon.

@jtblin jtblin closed this Jun 19, 2016
@drpicox
Copy link
Author

drpicox commented Jun 19, 2016

Nice!

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

Successfully merging this pull request may close these issues.

2 participants