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

fix(cookieReader): safely access $document so it can be mocked #11388

Closed
wants to merge 1 commit into from
Closed

fix(cookieReader): safely access $document so it can be mocked #11388

wants to merge 1 commit into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Mar 21, 2015

Closes #11373

@shahata
Copy link
Contributor Author

shahata commented Mar 21, 2015

I'm really not sure that this is the right way to solve this. I'm thinking it might be a good idea to have some mock implementation of cookieReader/cookieWriter in angular-mocks which can be configured during tests similarly to $httpBackend. We used to have something similar in $browser, but it wasn't documented and it got refactored out during the ngCookies re-write (which is why this problem only occurred now)

@Narretz
Copy link
Contributor

Narretz commented Mar 30, 2015

@petebacondarwin probably comes up with a good idea how to solve this, he always does :)

return lastCookies;
};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably overkill. If the developer mocks out window and doesn't include a document then lots of stuff is going to break. Instead, how about we simply do

lastCookieString = rawDocument.cookie || '';

In the function below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually this is the only place that breaks, you can mock out $window and not include $document. See https://github.com/angular/angular.js/blob/master/test/ng/windowSpec.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also do something like this, if you prefer:

rawDocument = $document[0] || {};
lastCookieString = rawDocument.cookie || '';

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh! Mocking window without a document is surely crazy juice!
OK let's go with this version:

rawDocument = $document[0] || {};
lastCookieString = rawDocument.cookie || '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, amended.

@petebacondarwin
Copy link
Contributor

LGTM - feel free to merge

@shahata shahata closed this in a057e08 Apr 3, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lastCookieString.split error with httpBackend.flush()
4 participants