Skip to content

Conversation

blazzy
Copy link
Contributor

@blazzy blazzy commented Dec 7, 2018

No description provided.

Copy link
Member

@djih djih left a comment

Choose a reason for hiding this comment

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

LGTM

const domain = levels[i];
const opts = { domain: '.' + domain };

baseCookie.set(cname, 1, opts);
Copy link
Member

Choose a reason for hiding this comment

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

my only gripe is that this function seems to be a getter method but it's also setting things into the 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.

Yeah. It's ugly. It does clean up after itself though.

There might be a better way of doing this, but I don't want to stray too far from what top-domain's approach without having time to set up proper tests.

remove('amplitude_test');
_options.domain = domain;

return _options;
Copy link
Member

Choose a reason for hiding this comment

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

who uses this return value? we weren't returning it before

@@ -0,0 +1,5 @@
const getLocation = () => {
Copy link
Member

Choose a reason for hiding this comment

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

super minor: you could probably put this in the utils file instead of making a new module for it

@blazzy blazzy merged commit 40ff69f into master Feb 25, 2019
@blazzy blazzy deleted the yarn-audit branch February 25, 2019 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants