[PR discussion] Compatibility with browserify and webpack

[pechitook]

In order to make this work with webpack or browserify-powered apps I needed to change the "main" to index.js in package.json and create an index.js file with:

require('./segment');
module.exports = 'ngSegment';

This practice is used in many other angular packages, but I'm not sure it is safe to apply to this project. What do you think?

[pechitook]

As today, people who uses webpack with this package will have to make an alias in order to use the module

//webpack.config.js
resolve: {
      alias: {
          'angular-segment-analytics': 'angular-segment-analytics/segment.js'
      }
  }

and then require('angular-segment-analytics') (or import ... from), then finally include 'ngSegment' (as a string) as a dependency for the app:

require('angular-segment-analytics')

angular.module('app', ['ngSegment'])

What many webpack-compatible packages do is to do that for you: require and use the string. If the above index.js file were included in this package, people will simply have to do

var ngSegment = require('angular-segment-analytics')

// more readable/modular code, as we import and depend on a variable, not a string.
angular.module('app', [ngSegment])

[aleross]

Thanks for pointing this out @p4bloch, I haven't used webpack or browserify with an Angular app (only React) so I hadn't considered this.

I think if the main were changed to index.js without a check for whether a module loader is present it may break other non-module-based tasks like grunt-bower-concat and the equivalent gulp and/or npm tasks.

It would be nice to provide support for all cases:

• Grunt/Gulp + Bower/npm
• Webpack + CommonJS or AMD
• Browserify

Can you think of any other example angular modules that handle this with best practices?

[pechitook]

I am positive that Auth0 has many AngularJS seniors in there and mgonto (creator of restangular) maintains this project: angular-storage, which works in webpack with the index.js method.

On the other hand, angular-ui-router itself does not use an index.js file, but instead they use the dist directly. There are some checks at the beginning of the file which may be just what we need here. Check out their angular-ui-router.js

[aleross]

Thanks for the examples. It looks like the ui-router method is probably the best fit to retain backwards compatibility with projects that use concat-based script loading. MomentJS also uses a similar check, and has support for AMD: moment/moment.js.

typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
    typeof define === 'function' && define.amd ? define(factory) :
    global.moment = factory()

It would be safe to add a check like that to the top of src/module.js. Changing segment.js to index.js (and the package.json main reference) may also be a good idea because that seems to be the growing standard. I can do this in the next week or so, or if you want to open up a PR @p4bloch I'd be happy to merge it.

I can't think of any tests that need to be added. I'd like to convert the project to ES6 (+babel) / webpack in the future, which would implicitly test the CommonJS import.

[pechitook]

Glad you had the time to make this happen. Very appreciated :)