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

Abandon the use of $window #35

Merged
merged 1 commit into from
Mar 16, 2014
Merged

Abandon the use of $window #35

merged 1 commit into from
Mar 16, 2014

Conversation

just-boris
Copy link
Contributor

It would be better to make moment() function injectable one time instead of finding it in $window every time.
Moreover, I've used $timeout instead of raw $window methods.

It would be better to make moment() function injectable one time instead of finding it in $window every time.
Moreover, I've used $timeout instead of raw $window methods.
@yaru22
Copy link

yaru22 commented Mar 5, 2014

I like the changes (getting rid of $window) in this CL. Also, I think it's great that he's using $timeout service now as explained in #25. @urish, would you be able to merge this change?

@urish
Copy link
Owner

urish commented Mar 16, 2014

Thank you @just-boris, well done!

Making moment() injectable does make a lot of sense, and I believe it is also going to make the implementation of #36 (require.js support) much more straight forward. Will merge.

However, using $timeout is problematic since:

  1. It will break e2e tests that rely on protractor's waitForAngular() method. Since this is the default behavior in protractor, this will likely impact anyone using protractor for e2e testing (see Protractor never synchronizes on pages using am-time-ago directive #19).
  2. It will cause a digest cycle every time we run the updateTime() method. This can be very expensive if we have multiple instance on a single page (see Handle null data #8).

One way to get around these issues is to use $interval with the the invokeApply argument set to false.

Are there any strong arguments in favor of using $interval instead of $window.setTimeout ?

urish added a commit that referenced this pull request Mar 16, 2014
@urish urish merged commit 3ad66eb into urish:master Mar 16, 2014
@just-boris
Copy link
Contributor Author

I just didn't know about $interval, sorry. Certainly, I will rewrite using it

@urish
Copy link
Owner

urish commented Mar 16, 2014

Hi @just-boris, thanks. One issue with $interval is that it is only available since angular.js 1.2.0, so we will have to drop support for angular 1.0.x users if we choose this approach. What is the advantage of not using $window in angular-moment code?

@just-boris
Copy link
Contributor Author

I hope that can make testing more simpy. If not, the second way is a using of jasmine's clock's mock.
I try to experiment with this and let you know the result.

@urish
Copy link
Owner

urish commented Mar 16, 2014

The clock mock sounds like a great idea... Looking forward to seeing your findings :-)

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

3 participants