urish / angular-moment

Moment.JS directives for Angular.JS (timeago and more)
MIT License
2.6k stars 397 forks source link

Change from moment dependency to moment-timezone #234

Closed kylecannon closed 8 years ago

kylecannon commented 8 years ago

moment-timezone replaces moment when installed via npm (installation instructions here). Due to the require statements asking for moment instead of moment-timezone, it's impossible to get moment.tz to be defined. Changing the package.json dependency from moment to moment-timezone brings back expected functionality when using tools such as browserify and webpack.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 92.627% when pulling e19e37a91c2a9cde11625a125fe46e9e796bec8a on ISITE-Software:master into c3f3361e49fc057ab38ad01f132c51a6037628f7 on urish:master.

urish commented 8 years ago

Hi, thanks for the contribution!

This going to break a lot of existing code (not everyone uses browserify or webpack).

Did you try requiring moment-timezone before requiring angular-moment and see if this does the trick?

kylecannon commented 8 years ago

I did. I've spent about two hours or more trying to come up with a workaround before coming to this solution. However, if others aren't webpack or browserify this doesn't affect them at all as it doesn't change the global injection (only amd and commonJS style)

The tests still pass that were written for the global style includes since it's injected via a script tag.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 92.627% when pulling 8da25267d1b8d8c8ff9f2d774f08366da5bd51ce on ISITE-Software:master into c3f3361e49fc057ab38ad01f132c51a6037628f7 on urish:master.

kylecannon commented 8 years ago

The issue happens on Line 719 and Line 721 which implicitly requires moment instead of moment-timezone.

urish commented 8 years ago

There are also require.js users. I believe this can still be worked around by using angular.js dependency injection and overriding the moment constant with a suitable implementation (e.g. the one returned by requiring moment-timezone).

Can you please have a look at it and see if this does the trick?

kylecannon commented 8 years ago

Well don't I feel like an idiot... one... simple... line of code.

Thank you!

Also, for some weird reason beta 5 doesn't throw the error that I am seeing in beta 4. So I will upgrade to beta 5 and also keep ngModule.constant('moment', require('moment-timezone')); there as well incase.

kylecannon commented 8 years ago

Also, I appreciate the help and the fast response. Great library!

urish commented 8 years ago

Thanks Kyle!

Any chance you can send a PR with an update to README.md in favor of future users with the same need?

kylecannon commented 8 years ago

235

monove commented 5 years ago

Is there any way of doing this without require?