Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Change from moment dependency to moment-timezone #234

Closed
wants to merge 2 commits into from
Closed

Change from moment dependency to moment-timezone #234

wants to merge 2 commits into from

Conversation

kylecannon
Copy link
Contributor

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
Copy link

Coverage Status

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

@urish
Copy link
Owner

urish commented Mar 22, 2016

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
Copy link
Contributor Author

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
Copy link

Coverage Status

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

@kylecannon
Copy link
Contributor Author

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

@urish
Copy link
Owner

urish commented Mar 22, 2016

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
Copy link
Contributor Author

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 kylecannon closed this Mar 22, 2016
@kylecannon
Copy link
Contributor Author

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

@urish
Copy link
Owner

urish commented Mar 22, 2016

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
Copy link
Contributor Author

#235

@monove
Copy link

monove commented Oct 17, 2018

Is there any way of doing this without require?

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.

None yet

4 participants