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

Change from moment dependency to moment-timezone - #234

Closed
kylecannon wants to merge 2 commits into
masterfrom
unknown repository
Closed

Change from moment dependency to moment-timezone#234
kylecannon wants to merge 2 commits into
masterfrom
unknown repository

Conversation

@kylecannon

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

Coverage Status

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

@urish

urish commented Mar 22, 2016

Copy link
Copy Markdown
Owner

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

Coverage Status

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

@kylecannon

Copy link
Copy Markdown
Contributor Author

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

@urish

urish commented Mar 22, 2016

Copy link
Copy Markdown
Owner

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

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

@urish

urish commented Mar 22, 2016

Copy link
Copy Markdown
Owner

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

#235

@monove

monove commented Oct 17, 2018

Copy link
Copy Markdown

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.

4 participants