-
Notifications
You must be signed in to change notification settings - Fork 632
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
Add CommonJS support in accordance w/ Angular convention #567
Conversation
Thanks, Ben! Let me find some time to review and see if we can get this in 1.0 |
IMO, this is a no brainer. It is now the official way angular exports modules for CommonJS consumption. The sooner I can stop shimming these, the better. |
@@ -23,7 +23,7 @@ | |||
"firebase", | |||
"realtime" | |||
], | |||
"main": "dist/angularfire.js", | |||
"main": "index.js", | |||
"files": [ | |||
"dist/**", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also list index.js
in the files
list right here. It may not be required since npm may automatically include the file listed in main
, but I'd prefer to see it listed in files
as well.
Yeah, I mean this looks like goodness to me. I made one comment which I'd like to see addressed but then I'm a thumbs up on merging this in. |
Actually, now that I think about it, I'm not sure this is compatible with the fact that we got rid of |
Ignore my last comment; not thinking properly today. The module is still called |
Totally right on the need to include it in This requires much less of a hack and dries up your modules when you do build with Browserify. Here's ES5: https://github.com/bendrucker/angular-stripe/blob/f93a874097b30c757410ac5d5050a020edc01590/src/index.js |
Looks good, Ben. Thanks. |
Add CommonJS support in accordance w/ Angular convention
Implements #566