Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($sce): do not assume $document[0] exists #9663

Merged
merged 3 commits into from
Oct 17, 2014
Merged

fix($sce): do not assume $document[0] exists #9663

merged 3 commits into from
Oct 17, 2014

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Oct 17, 2014

this is important so that people can mock $window without having to add stuff that angular uses internally into it

Closes #9661

@petebacondarwin
Copy link
Contributor

I feel a little uncomfortable modifying this way.
How about we change it to use msie instead? Especially now we use DocumentMode to set it any way.

@shahata
Copy link
Contributor Author

shahata commented Oct 17, 2014

The problem with using msie is that it will break the unit tests that expects this error to happen. We could fiddle with the msie during this test to force this to happen, but I feel more uncomfortable about that... Another option is to add an option to mock msie, but I'm not sure it is worth it since this is the only place we need it. (see the discussion in the original issue)

@shahata
Copy link
Contributor Author

shahata commented Oct 17, 2014

Also, the way I handled it is consistent with what we do in $SnifferProvider

@gkalpak
Copy link
Member

gkalpak commented Oct 17, 2014

@shahata : What do you mean by "this is the only place we need it" ?
It is used in bunch of places; this was just the only place that didn't use msie directly, but tried to use $document[0].documentMode.
This seems rather inconsistent (to me). I would prefer to always determine IE version in the same way (i.e. grobal msie) and preferably also have a way to mock it.

Why don't we use $sniffer.msie (as we did previously) anyway ?

@shahata
Copy link
Contributor Author

shahata commented Oct 17, 2014

I mean this is the only place we need to mock msie, so I'm not sure if it is worth to add something like $sniffer.msie just for this issue. @petebacondarwin WDYT? I can easily add support for mocking msie to this PR if it makes sense.

@petebacondarwin
Copy link
Contributor

I would rather use msie and change the tests that mock it. Keep all the msie stuff in one place.

this is important so that people can mock $window without having to add stuff that angular uses internally into it

Closes #9661
* documentMode is an IE-only property
* http://msdn.microsoft.com/en-us/library/ie/cc196988(v=vs.85).aspx
*/
var msie = document.documentMode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need this as ngScenario gets compiled up into a separate file with its own closure.
It just happens that our unit tests run with all the files in a single closure! (Which is bad but no one has the energy to fix it)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$window mocking not working without document since 1.3.0
5 participants