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

Fixes error that occurs when local storage is enabled. #62

Merged
merged 3 commits into from
Aug 8, 2019

Conversation

bsneed
Copy link
Contributor

@bsneed bsneed commented Aug 5, 2019

  • Fixes error that occurs when local storage is disabled.
  • Unit test added to verify.

@bsneed bsneed requested review from fathyb and Peripheral1994 August 5, 2019 20:50
Copy link
Contributor

@fathyb fathyb left a comment

Choose a reason for hiding this comment

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

Could we add tests to prevent any regressions?

I don't think we usually bump the version in a PR, I think this will prevent the tag v4.2.1 from being created. Could you revert this commit?

@bsneed bsneed force-pushed the bsneed/fix-localstorage branch from e4e5db7 to 0fc1b58 Compare August 5, 2019 21:06
@f2prateek
Copy link
Contributor

To confirm, did you mean this error occurs when when local storage is disabled?

Agree a unit test would be good to have here!

@bsneed bsneed requested review from fathyb and f2prateek August 6, 2019 23:43
it('identify should not ultimately call getCachedCrossDomainId if crossDomainAnalytics is not enabled', function() {
var wasCalled = false;
var oldCrossDomainCheck = segment.isCrossDomainAnalyticsEnabled;
segment.isCrossDomainAnalyticsEnabled = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think isCrossDomainAnalyticsEnabled needs to be mocked, it can be disabled with segment.options.crossDomainIdServers = []; (example here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

return false;
};
var oldGetCrossDomainId = segment.getCachedCrossDomainId;
segment.getCachedCrossDomainId = function() {
Copy link
Contributor

@f2prateek f2prateek Aug 7, 2019

Choose a reason for hiding this comment

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

Could/should we use sinon for this?

https://sinonjs.org/releases/v7.4.1/spy-call/

segment.options.crossDomainIdServers = [];

var getCachedCrossDomainIdSpy = sinon.spy(segment, "getCachedCrossDomainId");

segment.normalize({});

sinon.assert.notCalled(getCachedCrossDomainIdSpy);

segment.getCachedCrossDomainId.restore();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@bsneed bsneed merged commit c6d9155 into master Aug 8, 2019
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.

3 participants